Conversation
Remove last leftover from 7242606
The TSan CI workflow should pass -DTSAN_INSTRUMENT_ALL through CPPFLAGS rather than CFLAGS
gasche
left a comment
There was a problem hiding this comment.
This looks fine to me. I was a bit surprised by the fact that we now rely more on CPPFLAGS, but it makes sense.
|
Thanks! Yeah if you talk about the second commit, those really are
preprocessor flags so it was slightly incorrect to have them in CFLAGS.
One may believe that CPPFLAGS are kind of a subset of CFLAGS and that
it would thus not hurt to have something in CFLAGS rather than CPPFLAGS
but it's not always true, e.g. when we compute dependencies we only pass
the CPPFLAGS because it's a preprocessor thing, not the CFLAGS.
@gasche as much as I enjoy my PRs being merged promptly, I think it'd be
wise to wait for another pair of eyes to have a look to this one.
@MisterDA for instance and/or @shym.
|
MisterDA
left a comment
There was a problem hiding this comment.
I approve this PR too, based on GNU Make Catalogue of Rules:
- Compiling C programs:
$(CC) $(CPPFLAGS) $(CFLAGS) -c; - Linking a single object file:
$(CC) $(LDFLAGS) n.o $(LOADLIBES) $(LDLIBS)(LOADLIBESbeing deprecated).
|
Thanks @dustanddreams!
|
|
Thanks!
Do we have a contest for the PR that gets the biggest number of
approvals? ;-)
|
|
Just pushed bb119d1 to add names of reviewers to the Changes entry for this PR, apologies for having forgotten. |
|
Your Changes entry credits @shym with a review, did it occur outside github/ocaml? |
|
Gabriel Scherer (2024/05/27 07:51 -0700):
Your Changes entry credits @shym with a review, did it occur outside
github/ocaml?
Yes, sorry I didn't mention that explicitly.
|
|
A small unforeseen consequence is that we used to pass |
The present PR is a follow-up to both #9824 (Build system: honour the
CFLAGS and CPPFLAGS build variables) and #9837 (Build system: use
OC_CFLAGS and CFLAGS even during the link stage).
After #9824 was merged, Inria's
extra-checksCI job got brokenbecause the link stage needed the sanitizer-related flags, too.
The fix that was then provided in #9837 was actually not correct: rather
than using CFLAGS also during the link stage, which is not what GNU make does
by default, we should rather
duplicate the relevant flags into LDFLAGS.
This makes sense because even a flag with the same name does not
have the same semantics depending on whether it is in CFLAGS or LDFLAGS.
For the instrumentation flags, for instance, when in CFLAGS they request the
code to be compiled with instrumentation, whereas when in LDFLAGS they
request that the runtime support for instrumentation to be linked in, which
is of course related but still slightly different.
The reason why the flags intended to be in CFLAGS and those intended to be
in LDFLAGS often have the same name is to make it possible to do both
compile and link in one go, which we still were doing sometimes at the time
of #9824 and #9837 (e.g. for
checkstack) but do not do any longernowadays: our build system pays a lot of attention not to confuse
compile and link commands, be it for programs / libraries in C or in OCaml.
This PR opens the road to an improvement in the way TSan build flags are
handled, which will considerably speed-up the build of TSan-enabled
compilers. Once this PR is merged, we will also be able to bring #12589 to
its clean and ocrrect completion.