undefined reference to 'log10' about llvm-bazel HOT 18 CLOSED

What's the command you're running and what platform are you on?

from llvm-bazel.

I'd guess you're not passing --config=generic_clang or similar to specify the config, which means you're not getting the right C++ version

from llvm-bazel.

(log10 is since C++11: https://en.cppreference.com/w/cpp/numeric/math/log10)

from llvm-bazel.

I'm running bazel build @llvm-project//..., but it's in our bazel monorepo at my company (Aurora Innovation) so it's difficult to share the exact configuration. I'm essentially certain it's not to do with the C++ version.

If I inspect the command line that fails (bazel-out/host/bin/external/llvm-project/mlir/mlir-linalg-ods-yaml-gen-2.params) I don't see -lm.

$ tail bazel-out/host/bin/external/llvm-project/mlir/mlir-linalg-ods-yaml-gen-2.params
bazel-out/host/bin/external/llvm-project/llvm/_objs/Support/xxhash.o
--end-lib
--start-lib
bazel-out/host/bin/external/llvm-project/llvm/_objs/Demangle/Demangle.o
bazel-out/host/bin/external/llvm-project/llvm/_objs/Demangle/ItaniumDemangle.o
bazel-out/host/bin/external/llvm-project/llvm/_objs/Demangle/MicrosoftDemangle.o
bazel-out/host/bin/external/llvm-project/llvm/_objs/Demangle/MicrosoftDemangleNodes.o
--end-lib
-ldl
-lstdc++

I guess I should pull down your exact bazel config in the example and see if that fails.

from llvm-bazel.

Ah also since this is a linker error, you're probably right that it's not a version issue

I've pattern matched too hard on "can't find thing in std -> missing c++ version specification"

from llvm-bazel.

I think you're right that we're just missing -lm here. We've already got it on mlir-linalg-ods-gen. I honestly don't understand why this gets automatically detected on some platforms and not others. Are there any other binaries that need it?

from llvm-bazel.

Yes, but not on mlir-linalg-ods-yaml-gen:

I'm speculating here, but should -lm actually be on the library rather than the binary target? Like here:

from llvm-bazel.

Given that your error is coming from llvm Support, I'd guess the answer is ~"all of them" 😛 I then wonder if we shouldn't just add this to Support itself. @chandlerc thoughts? Similarly, we've got -lpthread on a bunch of the binaries, but IIUC, having -pthread as we do in the llvm Support library should already mean that everything transitively has that and -pthread is a superset of -lpthread. It seems like we're sticking these options at leaves when they should be in the library that actually needs it

from llvm-bazel.

Ah, seems we maybe came to that conclusion at the same time :)

Yes I think -lm should be on the Support target and would be inherited (if I understand linkopts correctly). Because the log10 call is in that library.

https://github.com/llvm/llvm-project/blob/2f69975683f5b6ea7df79f335f96b889a4bddecd/llvm/lib/Support/Signals.cpp#L196

from llvm-bazel.

Can you test if add -lm to llvm:Support fixes your issue? I'll send a PR dropping all the linkopts from the leaf binaries and adding -lm to llvm:Support, which I think is the right thing to do. There will be some weirdness by nature of this being a transitive thing (line includes) that means we might have some things happen to work because they dep on Support even though there are other libraries that really need these options and don't have them, but I'm not sure how to avoid that

from llvm-bazel.

(I can't test because for reasons that remain unclear to me this doesn't fail on my machine. Something is magically figuring out it needs math and pulling it in)

from llvm-bazel.

FWIW, adding -lm sounds fine to me.

Arguably, Bazel's C++ toolchain might should do this for us since it is part of C/C++. But there's no reason to stress about why that isn't happening, adding it to our target seems reasonable.

from llvm-bazel.

Great, sent you a PR :-)

We could also add this to linkopts (but that would require @mmcloughlin also adding it to the linkopts of their toolchain)

from llvm-bazel.

For what it's worth, just adding --host_linkopt=-lm to my command line appears to fix everything for me.

I suspect #189 should be enough. I'll let you know if not.

As for why this was magically working for you, I'm wondering if -lm is an automatic default dependency in your toolchain/stdlib. Something like hinted in this answer: https://stackoverflow.com/a/1033940.

from llvm-bazel.

Yeah that makes sense. It's not clear to me whether the "right" way to do this is to add it to libraries or just say it has to be part of the toolchain. This seems fine for now though 🙂 thanks for helping root out this issue.

from llvm-bazel.

Well, my instinct is saying this was working "by accident" for you before. I think moving the -lm is correct. It would just be reassuring to repro this in the CI system to ensure similar issues are caught in future.

Anyway, thanks for acting so fast! We had our own in-house bazel rules before and it was nasty. This repo is a far better solution.

from llvm-bazel.

Yes I agree this was working by accident before. I guess my question was whether it's better to add -lm to a specific library that needs it or just declare this part of the standard library to be required (in the same way we say c++14 is required) at the level of the toolchain. The fact that some toolchains seem to already be doing this for us muddied things a bit 😁

Anyway, thanks for acting so fast! We had our own in-house bazel rules before and it was nasty. This repo is a far better solution.

We have like 10 different copies of in-house bazel rules, so it's even better 😛 FYI the contents of this repo will shortly (ish) be moving into the llvm monorepo proper (https://github.com/llvm/llvm-www/blob/main/proposals/LP0002-BazelBuildConfiguration.md). The delay from my end has been making sure I have folks at Google aligned so we can quickly delete our various internal copies when that happens.

So heads up that the precise way to integrate with this will change a little bit (though should just get easier)

from llvm-bazel.

Awesome. In-tree will be even better 😃

from llvm-bazel.