Use LLVM_VERSION_MAJOR from llvm-config.h instead of inferred LLVM_MAJOR_VERSION #4737
Conversation
chantra
requested review from
drzaeus77,
yonghong-song,
4ast and
davemarchevsky
as code owners
chantra
force-pushed
the
llvm_config
branch
from
27c1361
to
18b2fef
Compare
chantra
force-pushed
the
llvm_config
branch
from
18b2fef
to
bb60f5a
Compare
…JOR_VERSION Instead of inferring this from within the CMakeList.txt file, we can pull it directly from llvm-config.h.
chantra
force-pushed
the
llvm_config
branch
from
bb60f5a
to
6f11bf7
Compare
|
hum... I am not sure those errors are related. Forcing an update to re-trigger build. |
|
ok, tests that previously failed, now succeed. Tests that previously succeeded, now fail. |
| @@ -19,7 +19,7 @@ | |||
| #include <tuple> | |||
| #include <vector> | |||
|
|
|||
| #if LLVM_MAJOR_VERSION >= 15 | |||
There was a problem hiding this comment.
Too much code churn, could we avoid this change here ?
There was a problem hiding this comment.
@chenhengqi what do you mean by too much code churn?
This is essentially a sed s/LLVM_MAJOR_VERSION/LLVM_VERSION_MAJOR/ on the files, e.g from internally defined to the one provided by llvm itself.
The main reason for doing this is that when using alternate build systems such as buck, bazel... the LLVM version get automatically discovered at build time without needing some special handling by the build system.
Given that this is readily available, we can use it directly instead of re-inventing the wheel.
There was a problem hiding this comment.
without needing some special handling by the build system
But we still rely on cmake to find the LLVM libraries, no ?
There was a problem hiding this comment.
In a build agnostic system, the build system will find the llvm libraries (cmake, make, bazel, buck....), yes.
LLVM library itself (via llvm-config.h https://github.com/llvm-mirror/llvm/blob/2c4ca6832fa6b306ee6a7010bfb80a3f2596f824/include/llvm/Config/llvm-config.h.cmake#L69 ) exposes the major version (and more).
Currently, this is inferred from
Line 47 in ec49363
I am replacing this by llvm native defines.
In iovisor#4737 I added support for using LLVM_VERSION_MAJOR from llvm-config.h instead of inferred LLVM_MAJOR_VERSION. This went through CI just fine because CI uses LLVM 14 as the latest LLVM version (through the fedora 36 build). `bcc_debug.cc` was using LLVM_VERSION_MAJOR before any include that would actually define it, causing LLVM_VERSION_MAJOR to default to 0. This was fine for builds using LLVM < 15, but does not work for LLVM >= 15. On Ubuntu 23.04, using ``` -- Found LLVM: /usr/lib/llvm-15/include 15.0.7 (Use LLVM_ROOT envronment variable for another version of LLVM) ``` compilation of bcc_debug.cc would fail with: https://gist.github.com/chantra/d2277d500d150ec2cc863dd412bd3264 This change moves the include of `bpf_module.h` up the file to make sure LLVM_VERSION_MAJOR is defined. After this change, compilation goes through just fine. Signed-off-by: Manu Bretelle <chantr4@gmail.com>
In iovisor#4737 I added depedency on llvm-config.h in order to discover what LLVM version is being used. This was only meant to be used at build time. By adding llvm-config.h in bpf_module.h, a public header of libbcc, I introduced a dependency on llvm-dev to libbcc which is unnecessary. This change removes the include from `bpf_module.h`, which should be enough. It does not seem I need to add the include in any other files. Fixes iovisor#4997 Signed-off-by: Manu Bretelle <chantr4@gmail.com>
In iovisor#4737 I added depedency on llvm-config.h in order to discover what LLVM version is being used. This was only meant to be used at build time. By adding llvm-config.h in bpf_module.h, a public header of libbcc, I introduced a dependency on llvm-dev to libbcc which is unnecessary. This change removes the include from `bpf_module.h`, which should be enough. It does not seem I need to add the include in any other files. Fixes iovisor#4997 Signed-off-by: Manu Bretelle <chantr4@gmail.com>
In iovisor#4737 I added depedency on llvm-config.h in order to discover what LLVM version is being used. This was only meant to be used at build time. By adding llvm-config.h in bpf_module.h, a public header of libbcc, I introduced a dependency on llvm-dev to libbcc which is unnecessary. This change removes the include from `bpf_module.h`, which should be enough, and add it to `bpf_module.cc` instead. Fixes iovisor#4997 Signed-off-by: Manu Bretelle <chantr4@gmail.com>
In #4737 I added depedency on llvm-config.h in order to discover what LLVM version is being used. This was only meant to be used at build time. By adding llvm-config.h in bpf_module.h, a public header of libbcc, I introduced a dependency on llvm-dev to libbcc which is unnecessary. This change removes the include from `bpf_module.h`, which should be enough, and add it to `bpf_module.cc` instead. Fixes #4997 Signed-off-by: Manu Bretelle <chantr4@gmail.com>
Instead of inferring this from within the CMakeList.txt file, we can pull it directly from llvm-config.h.