◐ Shell
reader mode source ↗
Skip to content

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

Closed
vstinner wants to merge 1 commit into
python:mainfrom
vstinner:configure_asan
Closed

gh-109575: Don't export -fsanitize compiler options#109576
vstinner wants to merge 1 commit into
python:mainfrom
vstinner:configure_asan

Conversation

@vstinner

@vstinner vstinner commented Sep 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

vstinner commented Sep 19, 2023

Copy link
Copy Markdown
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

vstinner commented Sep 19, 2023

Copy link
Copy Markdown
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

vstinner commented Sep 19, 2023

Copy link
Copy Markdown
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

gpshead commented Sep 20, 2023

Copy link
Copy Markdown
Member

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

vstinner commented Oct 2, 2023

Copy link
Copy Markdown
Member Author

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