Skip to content

gh-109575: Don't export -fsanitize compiler options #109576

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

gh-109575: Don't export -fsanitize compiler options #109576

wants to merge 1 commit into from
+18 −13

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 19, 2023 •
edited
Loading

When Python is configured to use ASAN, MSAN or UBSAN sanitizer, with "./configure --with-address-sanitizer" for example, compiler and linker flags for sanitizers are no longer exported to third party C extensions. Add flags to CFLAGS_NODIST and LDFLAGS_NODIST, instead of BASECFLAGS and LDFLAGS.

Makefile.pre.in: PY_LDFLAGS_NOLTO now uses PY_LDFLAGS_NODIST, instead of LDFLAGS_NODIST, and add CONFIGURE_LDFLAGS_NOLTO after PY_LDFLAGS_NODIST.

@vstinner vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Sep 19, 2023
@bedevere-app bedevere-app bot mentioned this pull request Sep 19, 2023
Closed
@vstinner
Copy link
Member Author

On the main branch, with the bug:

$ ./configure --with-pydebug --with-address-sanitizer CC=clang 
$ grep fsanitize Makefile
BASECFLAGS=	-fsanitize=address -fno-omit-frame-pointer  -fno-strict-overflow -Wsign-compare -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer
CONFIGURE_LDFLAGS=	-fsanitize=address

With the fix, the flags are not longer exported:

$ ./configure --with-pydebug --with-address-sanitizer CC=clang 
$ grep fsanitize Makefile
CONFIGURE_CFLAGS_NODIST= -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden -fsanitize=address -fno-omit-frame-pointer
CONFIGURE_LDFLAGS_NODIST= -fsanitize=address

@vstinner
Copy link
Member Author

cc @gpshead @erlend-aasland

@vstinner
pythongh-109575: Don't export -fsanitize compiler options
a9da37c 
When Python is configured to use ASAN, MSAN or UBSAN sanitizer, with
"./configure --with-address-sanitizer" for example, compiler and
linker flags for sanitizers are no longer exported to third party C
extensions. Add flags to CFLAGS_NODIST and LDFLAGS_NODIST, instead of
BASECFLAGS and LDFLAGS.

Makefile.pre.in: PY_LDFLAGS_NOLTO now uses PY_LDFLAGS_NODIST, instead
of LDFLAGS_NODIST, and add CONFIGURE_LDFLAGS_NOLTO after
PY_LDFLAGS_NODIST.
@vstinner
Copy link
Member Author

Test PY_LDFLAGS_NOLTO on building _bootstrap_python:

./configure --with-address-sanitizer --with-lto
make

Logs:

gcc   -flto -fuse-linker-plugin -ffat-lto-objects -flto-partition=none -g -fsanitize=address  -fno-lto -o _bootstrap_python (...)

-fno-lto option is passed as expected.

@vstinner vstinner mentioned this pull request Sep 19, 2023
Merged
@gpshead
Copy link
Member

gpshead commented Sep 20, 2023

this change makes sense if we want to do this. it wasn't clear what "right" behavior was to me for this, see my comment on the Issue. (i'm happy either way myself)

@vstinner
Copy link
Member Author

vstinner commented Oct 2, 2023

I thought that it was a mistake, but apparently, some people actually want this behavior. I'm mostly using ASAN, but for ASAN, it seems to be the expected behavior. So I just close my PR :-)

If someone wants to adjust the code for other sanitizers, please go ahead!

@vstinner vstinner closed this Oct 2, 2023
@vstinner vstinner deleted the configure_asan branch October 2, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner

Assignees
No one assigned
Labels
awaiting review needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vstinner @gpshead