Add -MP to CFLAGS along with -MMD.

Commit Message

David Woodhouse March 17, 2020, 2:34 p.m. UTC

From: David Woodhouse <[e-mail]>

This causes gcc (yes, and clang) to emit phony targets for each dependency.

This means that when a header file is deleted, the C files which *used*
to include it will no longer stop building with bogus out-of-date
dependencies like this:

  make[5]: *** No rule to make target
  '/home/dwmw2/git/xen/xen/include/asm/hvm/svm/amd-iommu-proto.h',
  needed by 'p2m.o'. Stop.

Signed-off-by: David Woodhouse <[e-mail]>
---
 tools/Rules.mk | 2 +-
 xen/Rules.mk   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Andrew Cooper March 17, 2020, 2:39 p.m. UTC | #1

On 17/03/2020 14:34, David Woodhouse wrote:
> From: David Woodhouse <[e-mail]>
>
> This causes gcc (yes, and clang) to emit phony targets for each dependency.
>
> This means that when a header file is deleted, the C files which *used*
> to include it will no longer stop building with bogus out-of-date
> dependencies like this:
>
>   make[5]: *** No rule to make target
>   '/home/dwmw2/git/xen/xen/include/asm/hvm/svm/amd-iommu-proto.h',
>   needed by 'p2m.o'. Stop.
>
> Signed-off-by: David Woodhouse <[e-mail]>

Acked-by: Andrew Cooper <[e-mail]>

Jan Beulich March 17, 2020, 2:52 p.m. UTC | #2

On 17.03.2020 15:34, David Woodhouse wrote:
> From: David Woodhouse <[e-mail]>
> 
> This causes gcc (yes, and clang) to emit phony targets for each dependency.
> 
> This means that when a header file is deleted, the C files which *used*
> to include it will no longer stop building with bogus out-of-date
> dependencies like this:
> 
>   make[5]: *** No rule to make target
>   '/home/dwmw2/git/xen/xen/include/asm/hvm/svm/amd-iommu-proto.h',
>   needed by 'p2m.o'. Stop.

In principle this would be nice, but there must be a reason this isn't
the default behavior. As the workaround for the issue at hand is quite
simple, I wouldn't like to treat addressing this one by some other
anomaly/quirk. Do you (or does anyone else) have insight into why this
isn't default behavior?

Jan

David Woodhouse March 17, 2020, 3:19 p.m. UTC | #3

On Tue, 2020-03-17 at 15:52 +0100, Jan Beulich wrote:
> On 17.03.2020 15:34, David Woodhouse wrote:
> > From: David Woodhouse <[e-mail]>
> > 
> > This causes gcc (yes, and clang) to emit phony targets for each dependency.
> > 
> > This means that when a header file is deleted, the C files which *used*
> > to include it will no longer stop building with bogus out-of-date
> > dependencies like this:
> > 
> >    make[5]: *** No rule to make target
> >    '/home/dwmw2/git/xen/xen/include/asm/hvm/svm/amd-iommu-proto.h',
> >    needed by 'p2m.o'. Stop.
> 
> In principle this would be nice, but there must be a reason this isn't
> the default behavior. As the workaround for the issue at hand is quite
> simple, I wouldn't like to treat addressing this one by some other
> anomaly/quirk. Do you (or does anyone else) have insight into why this
> isn't default behavior?

No.

Ian Jackson March 17, 2020, 4:59 p.m. UTC | #4

David Woodhouse writes ("Re: [PATCH] Add -MP to CFLAGS along with -MMD."):
> On Tue, 2020-03-17 at 15:52 +0100, Jan Beulich wrote:
> > On 17.03.2020 15:34, David Woodhouse wrote:
> > > From: David Woodhouse <[e-mail]>
> > > 
> > > This causes gcc (yes, and clang) to emit phony targets for each dependency.
> > > 
> > > This means that when a header file is deleted, the C files which *used*
> > > to include it will no longer stop building with bogus out-of-date
> > > dependencies like this:
> > > 
> > >    make[5]: *** No rule to make target
> > >    '/home/dwmw2/git/xen/xen/include/asm/hvm/svm/amd-iommu-proto.h',
> > >    needed by 'p2m.o'. Stop.
> > 
> > In principle this would be nice, but there must be a reason this isn't
> > the default behavior. As the workaround for the issue at hand is quite
> > simple, I wouldn't like to treat addressing this one by some other
> > anomaly/quirk. Do you (or does anyone else) have insight into why this
> > isn't default behavior?
> 
> No.

I think this answer is:

I think it could interfere with other rules intended to build (or
rebuild) .h files.  This is particularly true for pattern or suffix
rules.  I would have to RTFM properly and think about it to understand
all the implications, to know what kind of nontrivial .h-rebuilding
rules might be affected (and therefore, to know whether we have any
such rules).

Sorry.

Ian.

David Woodhouse March 17, 2020, 5:12 p.m. UTC | #5

On Tue, 2020-03-17 at 16:59 +0000, Ian Jackson wrote:
> David Woodhouse writes ("Re: [PATCH] Add -MP to CFLAGS along with -MMD."):
> > On Tue, 2020-03-17 at 15:52 +0100, Jan Beulich wrote:
> > > On 17.03.2020 15:34, David Woodhouse wrote:
> > > > From: David Woodhouse <[e-mail]>
> > > > 
> > > > This causes gcc (yes, and clang) to emit phony targets for each dependency.
> > > > 
> > > > This means that when a header file is deleted, the C files which *used*
> > > > to include it will no longer stop building with bogus out-of-date
> > > > dependencies like this:
> > > > 
> > > >    make[5]: *** No rule to make target
> > > >    '/home/dwmw2/git/xen/xen/include/asm/hvm/svm/amd-iommu-proto.h',
> > > >    needed by 'p2m.o'. Stop.
> > > 
> > > In principle this would be nice, but there must be a reason this isn't
> > > the default behavior. As the workaround for the issue at hand is quite
> > > simple, I wouldn't like to treat addressing this one by some other
> > > anomaly/quirk. Do you (or does anyone else) have insight into why this
> > > isn't default behavior?
> > 
> > No.
> 
> I think this answer is:
> 
> I think it could interfere with other rules intended to build (or
> rebuild) .h files.  This is particularly true for pattern or suffix
> rules.  I would have to RTFM properly and think about it to understand
> all the implications, to know what kind of nontrivial .h-rebuilding
> rules might be affected (and therefore, to know whether we have any
> such rules).

That wasn't it.

David Woodhouse March 18, 2020, 6:12 p.m. UTC | #6

On Tue, 2020-03-17 at 16:59 +0000, Ian Jackson wrote:
> David Woodhouse writes ("Re: [PATCH] Add -MP to CFLAGS along with -MMD."):
> > On Tue, 2020-03-17 at 15:52 +0100, Jan Beulich wrote:
> > > On 17.03.2020 15:34, David Woodhouse wrote:
> > > > From: David Woodhouse <[e-mail]>
> > > > 
> > > > This causes gcc (yes, and clang) to emit phony targets for each dependency.
> > > > 
> > > > This means that when a header file is deleted, the C files which *used*
> > > > to include it will no longer stop building with bogus out-of-date
> > > > dependencies like this:
> > > > 
> > > >    make[5]: *** No rule to make target
> > > >    '/home/dwmw2/git/xen/xen/include/asm/hvm/svm/amd-iommu-proto.h',
> > > >    needed by 'p2m.o'. Stop.
> > > 
> > > In principle this would be nice, but there must be a reason this isn't
> > > the default behavior. As the workaround for the issue at hand is quite
> > > simple, I wouldn't like to treat addressing this one by some other
> > > anomaly/quirk. Do you (or does anyone else) have insight into why this
> > > isn't default behavior?
> > 
> > No.
> 
> I think this answer is:
> 
> I think it could interfere with other rules intended to build (or
> rebuild) .h files.  This is particularly true for pattern or suffix
> rules.  I would have to RTFM properly and think about it to understand
> all the implications, to know what kind of nontrivial .h-rebuilding
> rules might be affected (and therefore, to know whether we have any
> such rules).

OK... I have attempted to address my frustration in a more coherent and
hopefully productive way (qv), rather than resorting to monosyllabic
responses. Apologies for that.

Back to the specifics of this patch...

GCC has had -MD support since so far back that I can't even find its
origin in the git history. The SVN conversion seems kind of broken but
I see signs of -MD having existed as far back as 1992.

The -MP support, adding 'phony targets' for the headers listed in the
-MD output, wasn't added until 2001:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=a5a4ce3c3c0ee

It seems hardly surprising that the new behaviour was an additional
option to GCC instead of retroactively changing the default which had
already existed for around a decade. I do not think that questioning
the (understandably conservative) default behaviour of GCC is
appropriate or relevant in a review of a Xen patch such as this.

As it happens, the GCC documentation at 
https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html#index-MP
is slightly wrong. It refers to these as 'phony targets' but they aren't really 'phony targets' in the sense referred to in make docs at
https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html
because they aren't actually referenced from a .PHONY: rule.

Neither are they "empty recipes", as described further in
https://www.gnu.org/software/make/manual/html_node/Empty-Recipes.html
because empty recipes, well, aren't actually empty. Those would perhaps
more accurately be called "no-op recipes" because, like the example
there containing a single semicolon, they do nothing.

Both actual phony targets, and so-called empty recipes, do have the
effect of overriding implicit and pattern rules for the target in
question — they could mess up auto-generated header files, for example.

But what GCC -MP emits, despite its documentation, is neither of those
things. It merely emits an empty rule with no recipe and no actual
dependencies either. I don't think there's a specific term for that; it
*isn't* a 'phony rule', as I said.

It's probably best described as 'an explict rule without a recipe'.See 
https://www.gnu.org/software/make/manual/html_node/Multiple-Rules.html
where it states that

	'If none of the explicit rules for a target has a recipe, then 
	 make searches for an applicable implicit rule to find one
	 see Using Implicit Rules).'

So no, I don't think using -MP is going to break our handling of auto-
generated header files, but we'd have known that from a trivial
empirical build test within seconds, wouldn't we?

Here's another cut down test case. You can experiment with turning it
into a *real* 'phony rule', etc...

 $ grep ^ *
foo.c:#include <stdio.h>
foo.c:
foo.c:#include "foo.h"
foo.c:
foo.c:int main(void)
foo.c:{
foo.c:	printf(HELLO);
foo.c:	return 0;
foo.c:}
foo.h.orig:#define HELLO "Hello World!"
Makefile:#.PHONY: foo.h
Makefile:
Makefile:foo: foo.h foo.c
Makefile:	$(CC) -o foo foo.c
Makefile:
Makefile:%.h: %.h.orig
Makefile:	cp $< $@
Makefile:
Makefile:foo.h:
 $ make foo
cp foo.h.orig foo.h
cc -o foo foo.c

Ian Jackson March 18, 2020, 6:19 p.m. UTC | #7

David Woodhouse writes ("Re: [PATCH] Add -MP to CFLAGS along with -MMD."):
> OK... I have attempted to address my frustration in a more coherent and
> hopefully productive way (qv), rather than resorting to monosyllabic
> responses. Apologies for that.
> 
> Back to the specifics of this patch...

Wow.  Impressive.  Thank you for the comprehensive explanation.

Supposing you put all that in the commit message:
  Acked-by: Ian Jackson <[e-mail]>
(for the original patch)

If you were feeling dynamic, getting the gcc does (and maybe the make
docs) improved would be nice.

> So no, I don't think using -MP is going to break our handling of auto-
> generated header files, but we'd have known that from a trivial
> empirical build test within seconds, wouldn't we?

Unfortuantely in these days of many-core cpus, empirical tests showing
that the makefile works this time do not necessarily mean it will work
every time.  Maybe that wasn't a concern in this case, but my
experience in general teaches me not to rely solely on tests other
than to answer very narrow questions.

Sorry that this was frustrating in this case, but I think that some of
the lossage from our (sometimes appalling) makefiles has arisen due to
patches being committed that appeared to work at the time.  So I don't
think your research was wasted effort.

Anyway, I applaud your efforts to improve our Makefiles.

Regards,
Ian.

David Woodhouse March 18, 2020, 7:45 p.m. UTC | #8

On Wed, 2020-03-18 at 18:19 +0000, Ian Jackson wrote:
> David Woodhouse writes ("Re: [PATCH] Add -MP to CFLAGS along with -MMD."):
> > OK... I have attempted to address my frustration in a more coherent and
> > hopefully productive way (qv), rather than resorting to monosyllabic
> > responses. Apologies for that.
> > 
> > Back to the specifics of this patch...
> 
> Wow.  Impressive.  Thank you for the comprehensive explanation.

Actually, I think I may have erred. The make documentation at
https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html
makes a distinction between "phony targets" and ".PHONY targets".

What GCC emits *is* the former, while it is the latter which are
documented as causing implicit rules to be skipped.

So I think the GCC and Make documentation is entirely consistent, if
somewhat suboptimal in its use of terms with different meanings which
differ only in case and punctuation. Although looking back in
retrospect, the difference between "phony target" and ".PHONY target"
is clear enough that I wonder how I missed it.

> Supposing you put all that in the commit message:
>   Acked-by: Ian Jackson <[e-mail]>
> (for the original patch)

To be honest, I don't think it lives there. It's just a digression,
based on a half-misremembered idea that phony (not .PHONY) targets
cause implicit rules to be skipped.

Aside from that misremembrance, .PHONY rules aren't actually relevant
to this commit in any way.

It's two fairly pointless hours of my life that I want back, in which
I could have been doing something more useful on extending our live
update support to support inheriting the crashdump setup from the
previous Xen, as I had planned to do today.

Let's suppose I were to distil my 'research', including the above
correction, into an assertion along the lines of

"While make will skip implicit/pattern rules for targets which are 
 explicitly declared as .PHONY, what GCC emits with -MP are merely
 'phony targets' which are rules without a recipe, and which don't
 cause implicit rules to be skipped. Thus, the presence of such a
 rule should not prevent auto-generated header files and the like
 from being created correctly."

As we look back at this commit in future perhaps if we suspect it of
doing something wrong, what *benefit* is there to seeing that in the
commit comment?

Assuming it's a correct assertion, it's fairly much going to be
irrelevant to anyone who looks at this commit in future.

And if it's false, and I'm wrong? It's going to be either a red herring
derailing the investigation into what went wrong, or it only serves to
demonstrate how wrong I was :)

AFAICT there are very few situations, if any, in which anyone would
look back at this commit and find a comment about .PHONY targets to be
beneficial instead of just added noise.

It doesn't meet my criteria for being added to the commit comment.

May I have your Acked-By: without the addition, please?

> If you were feeling dynamic, getting the gcc does (and maybe the make
> docs) improved would be nice.

I was filing that GCC PR when I came to the realisation with which I
opened this email, and abandoned it.

> > So no, I don't think using -MP is going to break our handling of auto-
> > generated header files, but we'd have known that from a trivial
> > empirical build test within seconds, wouldn't we?
> 
> Unfortuantely in these days of many-core cpus, empirical tests showing
> that the makefile works this time do not necessarily mean it will work
> every time.  Maybe that wasn't a concern in this case, but my
> experience in general teaches me not to rely solely on tests other
> than to answer very narrow questions.

Indeed, but as you suggest I don't think this is a case that depends on
parallelism. Make tends to be fairly deterministic about what rules
*mean*, even when race conditions exist in parallel execution of those
rules.

> Sorry that this was frustrating in this case, but I think that some of
> the lossage from our (sometimes appalling) makefiles has arisen due to
> patches being committed that appeared to work at the time.  So I don't
> think your research was wasted effort.

Well, I refuted^Wrebutted *one* theory, which turns out to be
incorrect, about *one* way in which it might fail. This is far from
being a formal proof of correctness of my patch.

I suppose 'wasted' is a very subjective term and let's not argue over
whether it was wasted or not. But equally, we are no further forward
than we were before I had an erroneous theory to dispute.

Really no further forward in any way, because I didn't get to spend
that time doing anything more useful either.

Patch

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 52f47be3f8..9bac15c8d1 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -186,7 +186,7 @@  SHLIB_libxlutil  = $(SHDEPS_libxlutil) -Wl,-rpath-link=$(XEN_XLUTIL)
 CFLAGS += -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__
 
 # Get gcc to generate the dependencies for us.
-CFLAGS += -MMD -MF .$(if $(filter-out .,$(@D)),$(subst /,@,$(@D))@)$(@F).d
+CFLAGS += -MMD -MP -MF .$(if $(filter-out .,$(@D)),$(subst /,@,$(@D))@)$(@F).d
 DEPS = .*.d
 
 ifneq ($(FILE_OFFSET_BITS),)
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 92a13ca601..9079df7978 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -71,7 +71,7 @@  AFLAGS += -D__ASSEMBLY__
 ALL_OBJS := $(ALL_OBJS-y)
 
 # Get gcc to generate the dependencies for us.
-CFLAGS-y += -MMD -MF $(@D)/.$(@F).d
+CFLAGS-y += -MMD -MP -MF $(@D)/.$(@F).d
 
 CFLAGS += $(CFLAGS-y)
 # allow extra CFLAGS externally via EXTRA_CFLAGS_XEN_CORE