Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

llvm: add NDEBUG when assertion mode is off #45437

Merged
merged 2 commits into from
Jun 10, 2022

Conversation

haampie
Copy link
Contributor

@haampie haampie commented May 24, 2022

llvm-config --cxxflags unfortunately does not return -DNDEBUG when LLVM is
built with it, but Julia is required to have this define when including LLVM
header files. I didn't check if llvm-config --cxxflags has changed at some
point in LLVM (I think so because Julia switched from explicit -DNDEBUG to
llvm-config --cxxflags in v0.2.0 ages ago [1]), but the llvm-config --assertion-mode
flag was introduced a little after Julia dropped explicit -DNDEBUG [2].

Found this when compiling Julia with PGO enabled, which simply fails to link
because of virtual functions LLVM guards behind ifdefs.

[1] d2f6a78
[2] llvm-mirror/llvm@0427957

`llvm-config --cxxflags` unfortunately does not set `-DNDEBUG`, which
Julia needs to set correctly when including LLVM header files.
@vchuravy vchuravy requested a review from vtjnash May 24, 2022 13:22
@haampie
Copy link
Contributor Author

haampie commented May 24, 2022

Hm, static analysis doesn't like this. It follows a path where Log2_64(0) is called which returns 63 - 64 -> 4294967295 and is then used as a shift. Is it a false positive or an actual issue?

@vtjnash
Copy link
Member

vtjnash commented May 24, 2022

SGTM. I believe LLVM removed this from the default flags sometime between versions 6 and 8 (Julia v1.3 and v1.4) without us noticing. Until then, we had src/julia_assert.h working to ensure this was working as intended.

vtjnash@arctic4:/data/vtjnash$ ./julia13/usr/tools/llvm-config --cflags
-I/data/vtjnash/julia13/usr/include   -fPIC -Werror=date-time -Wall -W -Wno-unused-parameter -Wwrite-strings -Wno-missing-field-initializers -pedantic -Wno-long-long -Wundef -Wno-comment -ffunction-sections -fdata-sections -O3 -DNDEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS

vtjnash@arctic4:/data/vtjnash$ ./julia13/usr/tools/llvm-config --version
6.0.1

vtjnash@arctic4:/data/vtjnash$ ./julia14/usr/tools/llvm-config --cflags
-I/data/vtjnash/julia14/usr/include  -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS

vtjnash@arctic4:/data/vtjnash$ ./julia14/usr/tools/llvm-config --version
8.0.1jl

@vtjnash
Copy link
Member

vtjnash commented May 24, 2022

You might need to add -UNDEBUG to the analyze-gc step flags to counteract this. Otherwise, it can end up losing invariant information that has been annotated, for the sake of static analysis, such as this assert:
https://llvm.org/doxygen/Alignment_8h_source.html#l00077

@ViralBShah ViralBShah added the building Build system, or building Julia or its dependencies label May 25, 2022
@vtjnash vtjnash added the priority This should be addressed urgently label Jun 9, 2022
@vtjnash
Copy link
Member

vtjnash commented Jun 9, 2022

bump?

@vchuravy vchuravy added the merge me PR is reviewed. Merge when all tests are passing label Jun 9, 2022
@vchuravy vchuravy merged commit 7bc26a3 into JuliaLang:master Jun 10, 2022
@vchuravy vchuravy added the backport 1.8 Change should be backported to release-1.8 label Jun 10, 2022
@haampie haampie deleted the fix/retrieve-llvm-NDEBUG branch June 10, 2022 07:57
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Jun 10, 2022
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies priority This should be addressed urgently
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants