patchMakefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic

The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to use auto-detection in [2]. Then when -Wpedantic support was added to DEVOPTS in [3] we started passing -Wpedantic in combination with -Werror to the compiler here.

This broke the auto-detection, but since we'd quieted it in [4] we didn't find out. It was emitting all of this on STDERR under GCC:

    /dev/null:1: error: ISO C forbids an empty translation unit
    [-Werror=pedantic]
    cc1: note: unrecognized command-line option
    ‘-Wno-pedantic-ms-format’ may have been intended to silence
    earlier diagnostics
    cc1: all warnings being treated as errors

Let's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like ALL_CFLAGS but without anything we add in config.mak.dev, and furthermore stop redirecting STDERR to /dev/null, this means that someone whose compiler doesn't support this will see this output, but also this new message:

    Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

It's also possible that some compilers will emit warnings but still give a zero exit code, anyone using a compiler like that will potentially get more verbose output from the Makefile until they set COMPUTE_HEADER_DEPENDENCIES=no. E.g. on AIX's xlc we'll now emit:

    /opt/IBM/xlc/13.1.3/bin/.orig/xlc: 1501-208 (S) command option D is missing a subargument
    Non-zero 40 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

And on Solaris with SunCC:

    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise
    cc: refused to overwrite input file by output file: /dev/null
    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise
    cc: refused to overwrite input file by output file: /dev/null
    Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

Both are quieted by setting COMPUTE_HEADER_DEPENDENCIES=no as suggested.

I considered piping the output and the exit code to a variable instead, but e.g. under GCC that would lose the coloring in the error messages.

1. f2fabbf76e4 (Teach Makefile to check header dependencies,
   2010-01-26)
2. 111ee18c31f (Makefile: Use computed header dependencies if the
   compiler supports it, 2011-08-18)
3. 729b3925ed9 (Makefile: add a DEVOPTS flag to get pedantic
   compilation, 2018-07-24)
4. 6a8cbc41bac (developer: enable pedantic by default, 2021-09-03)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

On Wed, Sep 22, 2021 at 3:38 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

It is my impression though that header dependencies computation is something that might only work in gcc and clang (because of its gcc compatibility), so shouldn't this be restricted to those compilers instead of forcing all others to error?

Also, why is this process run with DEVELOPER=1 to begin with, if we obviously don't need/want any compilations warnings?

Carlo

On Wed, Sep 22 2021, Carlo Arenas wrote:

I think it's better to just have those not supporting COMPUTE_HEADER_DEPENDENCIES define that, wanting to auto-detect things is what led to the current breakage.

I.e. our whitelisting of GCC and Clang will be out of date if some other compiler grows support for this, and even if we whitelist gcc and clang we'll need some script like detect-compiler etc.

Hrm? We run the COMPUTE_HEADER_DEPENDENCIES=yes process with and without DEVELOPER=1, it's just that DEVELOPER=1 breaks it since it turns on pedantic compilation flags now.

We want -Werror for the actual compilation under DEVELOPER=1, but a one-off command to see if a compiler has support for something is entirely different.

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Are the references correct? I am not seeing "quiet"ing in [4]. The redirection 2>&1 to cram error messages also to $(dep_check), hence making it impossible to match '0', was done in [2].

We did make the pedantic mode the default and pass both -pedantic and -Wpedantic after [4]. Before we had only -pedantic.

Hmmmmmph.  

I recentaly saw many .depend directories (not necessarily empty) left after "make distclean". After building on one branch, I often check out a different branch then run distclean on the new branch, so leftover build artifacts are not necessarily a bug in our Makefile, but the bug you found may explain it?

While I agree with your analysis of the problem, I cannot shake this nagging feeling that the proposed solution is barking up a wrong tree. After all, -pedantic and any other option that lets the compiler notice that it is being asked to compile an empty source can come directly from the end user (e.g. CC="gcc -pedantic" or as part of CFLAGS)---realization of which makes me wonder if it is essential to compile /dev/null for this check, or any reasonably syntactically correct program would do.

I wonder if the attached (with clean-up to remove the tracing cruft) would show us a better direction. It feeds a single line

	int dummy_for_dep_check;

C "program" from the standard input of the compiler to tackle the "you are not supposed to be compiling an empty compilation unit" problem in a more direct way.

 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Wed, Sep 22, 2021 at 09:08:00AM -0700, Junio C Hamano wrote:

That feels a bit like we're playing a game of chicken with the compiler in terms of what it may complain about. For example, sparse will complain:

  foo.c:1:5: warning: symbol 'dummy_for_dep_check' was not declared. Should it be static?

Might compilers ever learn to warn of the same thing?

I kind of like the simplicity of Ævar's approach. We want to know if the compiler can be invoked with options XYZ, so we do so. That should be largely independent of our cflags, and there's prior art in how we invoke it in the detect-compiler script.

So I'd argue we should go even simpler, like:

Jeff King <peff@peff.net> writes:

Certainly. That is the reason why I said "direction", not "solution", and I do not think it is beyond our capability to come up with a minimal "C program" that would be lint clean to make it as a part of the "solution".

For example, would sparse or compilers complain about this?

    extern int incr(int); int incr(int i) { return i + 1; }

I am all for this simplification. It takes us back to the state before 1816bf26 (Makefile: Improve compiler header dependency check, 2011-08-30). But I think that is more or less orthogonal to the "you are not supposed to feed an empty compilation unit" issue.

Yup, I like that, too.

On Wed, Sep 22, 2021 at 11:28 AM Junio C Hamano <gitster@pobox.com> wrote:

the problem really is IMHO, that we are passing around CFLAGS that indicate 2 things:

1) attributes that are relevant to how we build and what
2) which diagnostics we want to use

Ævar's approach addresses implicitly part of it (most of the diagnostics are added in config.mak.dev), but I think it still feels like a knee jerk reaction to the problem, while creating an maintaining something like "DIAGNOSTIC_FLAGS" which we will only pass to the compiler when we want to use diagnostic (including -pedantic, -Wpedantic and -Werror) would be a better approach.

Carlo

On Wed, Sep 22, 2021 at 11:28:38AM -0700, Junio C Hamano wrote:

Fair enough. It is a game of chicken, but it is one that is easy to win. :)

I almost suggested using "git.c" as the dummy file, since we know it must compile anyway. But that probably has other problems (it's more expensive, and if it _does_ have an error, the results may be confusing).

It's a shame we can't just try to do the _real_ compiles using the auto-dependency stuff, and then fall back if they fail. But I think there's a chicken-and-egg problem there with "make" doing real work, and figuring out the dependencies to do real work.

Hmm, my suggestion was off the cuff without digging to see whether we used to do something similar. ;)

I do worry a bit that we'd be regressing the case that commit tried to fix. OTOH, I'm not sure I understand its commit message. It talks about things in CFLAGS being a problem, but it looks like the original (and my proposal here) would not look at CFLAGS at all? If people are putting stuff into CC that will break when used without CFLAGS, then I feel like the answer might be "don't do that". Or are there common situations where $(CC) is not expected to behave sensibly on its own?

I'm happy to submit that on top, or even turn the earlier hunk into a patch. But let's see what Ævar has to say to what's been discussed so far. I don't want to derail his effort.

-Peff

On Wed, Sep 22, 2021 at 1:17 PM Jeff King <peff@peff.net> wrote:

Yes, the problem was ONLY with the gcc that came in macOS before they moved to clang and that had a frankenstein set of options which will prevent people building BOTH fat binaries and doing this header dependency computation at the same time.

Ironically, modern clang makes his use case even uglier (running on top of Ævar's fix) :

$ make CFLAGS="-arch x86_64 -arch arm64" fatal error: /Library/Developer/CommandLineTools/usr/bin/lipo: can't create temporary output file: /dev/null.lipo (Operation not permitted) clang: error: lipo command failed with exit code 1 (use -v to see invocation) fatal error: /Library/Developer/CommandLineTools/usr/bin/lipo: can't create temporary output file: /dev/null.lipo (Operation not permitted) clang: error: lipo command failed with exit code 1 (use -v to see invocation) Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" t

so the test fails, and COMPUTE_HEADER_DEPENDENCIES is disabled, but it shouldn't and works fine if the test is overridden with COMPUTE_HEADER_DEPENDENCIES=yes

so by removing the CFLAGS from that test, we will actually be fixing this use case as well IMHO.

Carlo

On Wed, Sep 22 2021, Jeff King wrote:

Isn't this just a chicken & egg problem because we've made it a chicken & egg problem? I.e. this WIP hack seems to work for me to avoid it:

===============================================================================

On Thu, Sep 23, 2021 at 12:40:04AM +0200, Ævar Arnfjörð Bjarmason wrote:

Possibly, but...

I don't think this last dependency is quite right. The .dep for foo.c must rely not only on foo.c, but also all of its recursive headers (if it includes foo.h and then foo.h changes to include bar.h, we need to update the dep file). So it really needs to depend on everything in its own .dep file, too.

I definitely think that's easier to reason about, but I'm not surprised you found that it was slower. To get a more apples-to-apples comparison, I compiled with -j1. I also use -O0 as my default, which makes the "real" compilation a relatively smaller portion. I got 47s without your patch, and 60s with. That seems non-trivial.

I also noticed that "make clean" built all the deps. I think that could be fixed, but we're piling fixes upon fixes.

So I'm not really sure what we're gaining by splitting this all up. Given that our original problem is just fixing the test for "do we support computed dependencies", I think we can solve that pretty easily and move on.

Yeah, AFAIK the current system is pretty robust and battle-tested. I'd be cautious of messing with it unless there's a big gain to be seen.

Keep in mind the worst case in not using the dependency graph is just that a header update in an incremental compile may cause some extra compilation. It's really not _that_ bad, and especially so for folks who aren't actively doing development.

-Peff

Jeff King <peff@peff.net> writes:

Yeah, if compiling any of the real sources is inexpensive enough, I would think that would be the happy way to go. Do we have a trivial source that almost never changes? Perhaps version.c (especially if we kick out two helper functions that do not really belong there)?

Yup.

On Wed, Sep 22, 2021 at 05:03:58PM -0700, Junio C Hamano wrote:

Perhaps. TBH, I find Ævar's latest patch to just add -Wno-pedantic to be the simplest and most obviously-correct fix.

-Peff

Jeff King <peff@peff.net> writes:

Yes, that is very much to-the-point. If we have trouble with being pedantic, forcing us not to be is a fine solution ;-)

On Wed, Sep 22 2021, Junio C Hamano wrote:

Yes it's incorrect, I meant [2]. I had this right in my head, just got the references wrong somehow,thanks.

*nod*

Yes, I'll update the commit message, the problem is that we'll empty the dep_dirs list if we're not *currently* making them, that's a logic error in a few places in the Makefile, i.e. conflating currently building X with wanting to clean X.

[Will respond to the rest with a re-roll and/or in other replies in-thread]

In this v2 I just added an unconditional -Wno-pedantic and omitted the change to spew error on STDERR. This more narrowly fixes the immediate issue and doesn't get into whether we should use /dev/null or whatever as input.

I then noticed that the same bug was present in "GENERATE_COMPILATION_DATABASE=yes", so there's now a 2nd patch to fix that.

Ævar Arnfjörð Bjarmason (2):
  Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with
    DEVOPTS=pedantic
  Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes

 Makefile | 2 ++
 1 file changed, 2 insertions(+)

Range-diff against v1:
1:  3ff8ea12bf3 ! 1:  31c871e9bf6 Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic
    @@ Commit message
         Makefile: make COMPUTE_HEADER_DEPENDENCIES=auto work with DEVOPTS=pedantic
     
         The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to
    -    use auto-detection in [2]. Then when -Wpedantic support was added to
    -    DEVOPTS in [3] we started passing -Wpedantic in combination with
    -    -Werror to the compiler here.
    +    use auto-detection in [2], that "auto" detection has always piped
    +    STDERR to /dev/null, so any failures on compilers that didn't support
    +    these GCC flags would silently fall back to
    +    "COMPUTE_HEADER_DEPENDENCIES=no".
     
    -    This broke the auto-detection, but since we'd quieted it in [4] we
    -    didn't find out. It was emitting all of this on STDERR under GCC:
    +    Later when -Wpedantic support was added to DEVOPTS in [3] we started
    +    passing -Wpedantic in combination with -Werror to the compiler
    +    here. Note (to the pedantic): [3] actually passed "-pedantic", but it
    +    and "-Wpedantic" are synonyms.
    +
    +    Turning on -Wpedantic in [3] broke the auto-detection, since this
    +    relies on compiling an empty program. GCC would loudly complain on
    +    STDERR:
     
             /dev/null:1: error: ISO C forbids an empty translation unit
             [-Werror=pedantic]
    @@ Commit message
             earlier diagnostics
             cc1: all warnings being treated as errors
     
    -    Let's fix that bug by maintaining a NON_DEVELOPER_CFLAGS, it's like
    -    ALL_CFLAGS but without anything we add in config.mak.dev, and
    -    furthermore stop redirecting STDERR to /dev/null, this means that
    -    someone whose compiler doesn't support this will see this output, but
    -    also this new message:
    +    But as that ended up in the "$(dep_check)" variable due to the "2>&1"
    +    in [2] we didn't see it.
     
    -        Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect
    +    Then when [4] made DEVOPTS=pedantic the default specifying
    +    "DEVELOPER=1" would effectively set "COMPUTE_HEADER_DEPENDENCIES=no".
    +
    +    To fix these issues let's unconditionally pass -Wno-pedantic after
    +    $(ALL_CFLAGS), we might get a -Wpedantic via config.mak.dev after, or
    +    the builder might specify it via CFLAGS. In either case this will undo
    +    current and future problems with -Wpedantic.
     
    -    It's also possible that some compilers will emit warnings but still
    -    give a zero exit code, anyone using a compiler like that will
    -    potentially get more verbose output from the Makefile until they set
    -    COMPUTE_HEADER_DEPENDENCIES=no. E.g. on AIX's xlc we'll now emit:
    +    I think it would make sense to simply remove the "2>&1", it would mean
    +    that anyone using a non-GCC-like compiler would get warnings under
    +    COMPUTE_HEADER_DEPENDENCIES=auto, e.g on AIX's xlc would emit:
     
             /opt/IBM/xlc/13.1.3/bin/.orig/xlc: 1501-208 (S) command option D is missing a subargument
             Non-zero 40 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect
    @@ Commit message
             cc: refused to overwrite input file by output file: /dev/null
             Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect
     
    -    Both are quieted by setting COMPUTE_HEADER_DEPENDENCIES=no as
    -    suggested.
    -
    -    I considered piping the output and the exit code to a variable
    -    instead, but e.g. under GCC that would lose the coloring in the error
    -    messages.
    +    Both could be quieted by setting COMPUTE_HEADER_DEPENDENCIES=no
    +    explicitly, as suggested, but let's see if this'll fix it without
    +    emitting too much noise at those that aren't using "gcc" or "clang".
     
         1. f2fabbf76e4 (Teach Makefile to check header dependencies,
            2010-01-26)
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Makefile ##
    -@@ Makefile: ALL_COMMANDS_TO_INSTALL += git-upload-archive$(X)
    - ALL_COMMANDS_TO_INSTALL += git-upload-pack$(X)
    - endif
    - 
    --ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS)
    -+NON_DEVELOPER_CFLAGS = $(CPPFLAGS) $(CFLAGS)
    -+ALL_CFLAGS = $(DEVELOPER_CFLAGS) $(NON_DEVELOPER_CFLAGS)
    - ALL_LDFLAGS = $(LDFLAGS)
    - 
    - comma := ,
    -@@ Makefile: COMPUTE_HEADER_DEPENDENCIES = auto
    - endif
    +@@ Makefile: endif
      
      ifeq ($(COMPUTE_HEADER_DEPENDENCIES),auto)
    --dep_check = $(shell $(CC) $(ALL_CFLAGS) \
    -+dep_check = $(shell $(CC) $(NON_DEVELOPER_CFLAGS) \
    + dep_check = $(shell $(CC) $(ALL_CFLAGS) \
    ++	-Wno-pedantic \
      	-c -MF /dev/null -MQ /dev/null -MMD -MP \
    --	-x c /dev/null -o /dev/null 2>&1; \
    -+	-x c /dev/null -o /dev/null; \
    + 	-x c /dev/null -o /dev/null 2>&1; \
      	echo $$?)
    - ifeq ($(dep_check),0)
    - override COMPUTE_HEADER_DEPENDENCIES = yes
    - else
    -+$(info Non-zero $(dep_check) exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect)
    - override COMPUTE_HEADER_DEPENDENCIES = no
    - endif
    - endif
-:  ----------- > 2:  6b18bd08894 Makefile: pass -Wno-pendantic under GENERATE_COMPILATION_DATABASE=yes

The "COMPUTE_HEADER_DEPENDENCIES" feature added in [1] was extended to use auto-detection in [2], that "auto" detection has always piped STDERR to /dev/null, so any failures on compilers that didn't support these GCC flags would silently fall back to "COMPUTE_HEADER_DEPENDENCIES=no".

Later when -Wpedantic support was added to DEVOPTS in [3] we started passing -Wpedantic in combination with -Werror to the compiler here. Note (to the pedantic): [3] actually passed "-pedantic", but it and "-Wpedantic" are synonyms.

Turning on -Wpedantic in [3] broke the auto-detection, since this relies on compiling an empty program. GCC would loudly complain on STDERR:

    /dev/null:1: error: ISO C forbids an empty translation unit
    [-Werror=pedantic]
    cc1: note: unrecognized command-line option
    ‘-Wno-pedantic-ms-format’ may have been intended to silence
    earlier diagnostics
    cc1: all warnings being treated as errors

But as that ended up in the "$(dep_check)" variable due to the "2>&1" in [2] we didn't see it.

Then when [4] made DEVOPTS=pedantic the default specifying "DEVELOPER=1" would effectively set "COMPUTE_HEADER_DEPENDENCIES=no".

To fix these issues let's unconditionally pass -Wno-pedantic after $(ALL_CFLAGS), we might get a -Wpedantic via config.mak.dev after, or the builder might specify it via CFLAGS. In either case this will undo current and future problems with -Wpedantic.

I think it would make sense to simply remove the "2>&1", it would mean that anyone using a non-GCC-like compiler would get warnings under COMPUTE_HEADER_DEPENDENCIES=auto, e.g on AIX's xlc would emit:

    /opt/IBM/xlc/13.1.3/bin/.orig/xlc: 1501-208 (S) command option D is missing a subargument
    Non-zero 40 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

And on Solaris with SunCC:

    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise
    cc: refused to overwrite input file by output file: /dev/null
    cc: Warning: Option -x passed to ld, if ld is invoked, ignored otherwise
    cc: refused to overwrite input file by output file: /dev/null
    Non-zero 1 exit with COMPUTE_HEADER_DEPENDENCIES=auto, set it to "yes" or "no" to quiet auto-detect

Both could be quieted by setting COMPUTE_HEADER_DEPENDENCIES=no explicitly, as suggested, but let's see if this'll fix it without emitting too much noise at those that aren't using "gcc" or "clang".

1. f2fabbf76e4 (Teach Makefile to check header dependencies,
   2010-01-26)
2. 111ee18c31f (Makefile: Use computed header dependencies if the
   compiler supports it, 2011-08-18)
3. 729b3925ed9 (Makefile: add a DEVOPTS flag to get pedantic
   compilation, 2018-07-24)
4. 6a8cbc41bac (developer: enable pedantic by default, 2021-09-03)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

The same bug fixed in the "COMPUTE_HEADER_DEPENDENCIES=auto" mode in the preceding commit was also present with "GENERATE_COMPILATION_DATABASE=yes". Let's fix it so it works again with "DEVOPTS=1".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

FWIW a simpler fix was posted[1] earlier, and you were CC; sorry for the mess

Carlo

[1] https://lore.kernel.org/git/20210922185702.4328-1-carenas@gmail.com/

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Yup, I like reusing the same approach from the other fix; removing $(ALL_CFLAGS) may work fine, too, and that is what I already queued, but I may swap it out with this later (if I do not forget, that is).

On Thu, Sep 23, 2021 at 12:08:00AM +0200, Ævar Arnfjörð Bjarmason wrote:

FWIW, this seems perfectly fine to me. Removing $(ALL_CFLAGS) entirely seems OK to me, too, but this is a smaller change, and would help any cases where those flags are somehow important to getting the compiler to function at all.

It's possible some compiler _does_ understand -MF, etc, but not -Wno-pedantic, but that seems unlikely to me. (And of course there is always the fallback of setting COMPUTE_HEADER_DEPENDENCIES yourself on such a system; this is really just about doing the right thing on most people's setups).

Another related alternative is to use -Wno-error, which would fix the pedantic problem, along with any other warnings a compiler chooses to bring up for an empty file. I don't have a strong opinion on one versus the other.

-Peff