From: Junio C Hamano <hidden> Date: 2021-09-22 18:28:44
Jeff King [off-list ref] writes:
quoted
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.
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?
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.
quoted hunk
I'm also tempted by a hunk like this. Then we can set the REQUIRE flag
in a CI job (or locally for git devs who know they have gcc) and notice
an unexpected breakage in the auto test.
@@ -1295,6 +1295,9 @@ ifneq ($(COMPUTE_HEADER_DEPENDENCIES),no) $(error please set COMPUTE_HEADER_DEPENDENCIES to yes, no, or auto \ (not "$(COMPUTE_HEADER_DEPENDENCIES)")) endif+ifdef REQUIRE_COMPUTE_HEADER_DEPENDENCIES+$(error computed header dependencies required, but auto-check did not find them)+endif endif
From: Carlo Arenas <hidden> Date: 2021-09-22 18:44:16
On Wed, Sep 22, 2021 at 11:28 AM Junio C Hamano [off-list ref] wrote:
Jeff King [off-list ref] writes:
quoted
quoted
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.
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?
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.
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
From: Jeff King <hidden> Date: 2021-09-22 20:17:39
On Wed, Sep 22, 2021 at 11:28:38AM -0700, Junio C Hamano wrote:
quoted
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?
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; }
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.
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.
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?
quoted
I'm also tempted by a hunk like this. Then we can set the REQUIRE flag
in a CI job (or locally for git devs who know they have gcc) and notice
an unexpected breakage in the auto test.
@@ -1295,6 +1295,9 @@ ifneq ($(COMPUTE_HEADER_DEPENDENCIES),no) $(error please set COMPUTE_HEADER_DEPENDENCIES to yes, no, or auto \ (not "$(COMPUTE_HEADER_DEPENDENCIES)")) endif+ifdef REQUIRE_COMPUTE_HEADER_DEPENDENCIES+$(error computed header dependencies required, but auto-check did not find them)+endif endif
Yup, I like that, too.
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
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.
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?
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
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.
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:
===============================================================================
===============================================================================
This is just stealing a pattern we already use for
Documentation/doc.dep.
I.e. here every *.c file depends on a corresponding *.dep file, we
generate the *.dep files and *.c files seperately, instead of making the
*.dep a "while we're at it" as we make the *.o.
It's on obvious WIP, e.g. there's no ASM_DEP_OBJ yet, and e.g. a
"grep.dep" itself should in turn depend on say "strbuf.h", since if we
touch strbuf.h we should re-generate the grep.dep file before we make
the "grep.o" from the "grep.c".
It also means that if you have a clean tree and make "grep.o" we'll
generate all the *.dep files and the ALL.dep first, although skipping
that could be done with some clever use of $(MAKECMDGOALS) if anyone
cared.
It also makes "git clean -dxf; make -j8 all" take ~17s on my box instead
of ~13s.
I wonder if it's worth doing anyway, the cost of doing incremental
compiles isn't much affected.
I think the cases where the current schema bites us are rather obscure
though, but maybe I haven't thought of some obvious ones.
I.e. if we don't have the dep file we'll make it for the first time when
making the grep.o file, then include those generated dependencies.
If one of those dependencies changes we'll update the dependencies by
virtue of re-making the grep.o. I *think* the only edge cases are if
you've created a grep.o with COMPUTE_HEADER_DEPENDENCIES=yes, then
recompiled and it's gained a new dependency while using
COMPUTE_HEADER_DEPENDENCIES=no, and finally you try to recompile with
COMPUTE_HEADER_DEPENDENCIES=yes and it does nothing, because you're
referencing a stale ".depends" directory.
But that could be solved by just making the grep.o depend on its own
dependency file, which we don't do now, but could easily do.
So maybe I've talked myself out of there being an inherent dependency
graph violation with the current schema, i.e. the current one seems like
an easily solved bug.
One good thing the above approach still gives you is that you can use
GCC or Clang to make the dependency graph, but then use another compiler
to compile, i.e. we could have a $(GCC_LIKE) in addition to $(CC), so if
I'm compiling with xlc or suncc I can still use the present GCC just to
create the dependency graph.
From: Jeff King <hidden> Date: 2021-09-23 17:32:55
On Thu, Sep 23, 2021 at 12:40:04AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
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:
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.e. here every *.c file depends on a corresponding *.dep file, we
generate the *.dep files and *.c files seperately, instead of making the
*.dep a "while we're at it" as we make the *.o.
[...]
It also makes "git clean -dxf; make -j8 all" take ~17s on my box instead
of ~13s.
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.
So maybe I've talked myself out of there being an inherent dependency
graph violation with the current schema, i.e. the current one seems like
an easily solved bug.
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.
One good thing the above approach still gives you is that you can use
GCC or Clang to make the dependency graph, but then use another compiler
to compile, i.e. we could have a $(GCC_LIKE) in addition to $(CC), so if
I'm compiling with xlc or suncc I can still use the present GCC just to
create the dependency graph.
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