This is an archive of the discontinued LLVM Phabricator instance.

[C++20][Modules][Driver][HU 2/N] Add fmodule-header, fmodule-header=
ClosedPublic

Authored by iains on Mar 14 2022, 4:18 AM.

Details

Summary

These command-line flags are alternates to providing the -x
c++-*-header indicators that we are building a header unit.

Act on fmodule-header= for headers on the c/l:

If we have x.hh -fmodule-header, then we should treat that header
as a header unit input (equivalent to -xc++-header-unit-header x.hh).

Likewise, for fmodule-header={user,system} the source should be now
recognised as a header unit input (since this can affect the job list
that we need).

It's not practical to recognise a header without any suffix so
-fmodule-header=system foo isn't going to happen. Although
-fmodule-header=system foo.hh will work OK. However we can make it
work if the user indicates that the item without a suffix is a valid
header. (so -fmodule-header=system -xc++-header vector)

Diff Detail

Event Timeline

iains created this revision.Mar 14 2022, 4:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 4:18 AM
Herald added a subscriber: dang. · View Herald Transcript
iains published this revision for review.Mar 14 2022, 4:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 4:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

It's not practical to recognise a header without any suffix so

-fmodule-header=system foo isn't going to happen.

May I ask the reason? It looks not so good with -fmodule-header=system -xc++-header vector

iains added a comment.Mar 15 2022, 2:25 AM

It's not practical to recognise a header without any suffix so

-fmodule-header=system foo isn't going to happen.

May I ask the reason? It looks not so good with -fmodule-header=system -xc++-header vector

OK. One should never say "never" ;) , it would be nicer if we could avoid this.

... it would require a policy change in the driver - since we cannot recognise files like 'vector' as headers, they are currently unclaimed (which means that they default to being considered as linker inputs).

This is a long-standing (forever, I suspect) situation;
Although we could make it so that if we see certain options, all unknown inputs get claimed as source files (or headers) I wonder how much build system code that might break.

ChuanqiXu accepted this revision.Mar 15 2022, 3:28 AM

It's not practical to recognise a header without any suffix so

-fmodule-header=system foo isn't going to happen.

May I ask the reason? It looks not so good with -fmodule-header=system -xc++-header vector

OK. One should never say "never" ;) , it would be nicer if we could avoid this.

... it would require a policy change in the driver - since we cannot recognise files like 'vector' as headers, they are currently unclaimed (which means that they default to being considered as linker inputs).

This is a long-standing (forever, I suspect) situation;
Although we could make it so that if we see certain options, all unknown inputs get claimed as source files (or headers) I wonder how much build system code that might break.

If this is the policy, I am OK. For the build systems, due to the dependences, all build systems want to support C++20 modules should do works. So if we are doing our best, I think it might not be bad.

This revision is now accepted and ready to land.Mar 15 2022, 3:28 AM
iains added a comment.Mar 16 2022, 2:44 AM

It's not practical to recognise a header without any suffix so

-fmodule-header=system foo isn't going to happen.

May I ask the reason? It looks not so good with -fmodule-header=system -xc++-header vector

OK. One should never say "never" ;) , it would be nicer if we could avoid this.

... it would require a policy change in the driver - since we cannot recognise files like 'vector' as headers, they are currently unclaimed (which means that they default to being considered as linker inputs).

This is a long-standing (forever, I suspect) situation;
Although we could make it so that if we see certain options, all unknown inputs get claimed as source files (or headers) I wonder how much build system code that might break.

If this is the policy, I am OK. For the build systems, due to the dependences, all build systems want to support C++20 modules should do works. So if we are doing our best, I think it might not be bad.

So I had a chance to discuss this briefly with some of the other modules folks, and the consensus there was that changing the behaviour of the driver w.r.t linker inputs would likely cause build system problems, as I suggested above.
One possibility might be to find some shorter way (than -xc++-header) for the user to say that a file was a header (but IMO that is something for the future and there are interactions with shells to consider etc. - here we are trying to implement the functionality correctly and optimisation can follow later).

iains updated this revision to Diff 416854.Mar 21 2022, 1:39 AM

rebased.

iains updated this revision to Diff 418171.Mar 25 2022, 3:28 AM

rebased, fix testcase for windows, add command line options to docs.

iains updated this revision to Diff 418172.Mar 25 2022, 3:30 AM

fix some formatting.

iains updated this revision to Diff 418187.Mar 25 2022, 4:43 AM

amend filecheck match to avoid quoting windows pathnames.

iains updated this revision to Diff 418188.Mar 25 2022, 4:46 AM

amend second testcase for windows

iains updated this revision to Diff 418196.Mar 25 2022, 5:57 AM

another tweak to the second testcase for windows
*sigh* ... I will get the recipe right eventually.

iains updated this revision to Diff 418220.Mar 25 2022, 7:51 AM

we need the quotes around the components.

tahonermann added inline comments.Apr 25 2022, 11:45 AM
clang/docs/ClangCommandLineReference.rst
1187–1189

Are "user" and "system" the right terms to use here? Existing options aren't all that consistent. Some examples from Options.td:

def MD : Flag<["-"], "MD">, Group<M_Group>,
    HelpText<"Write a depfile containing user and system headers">;
def MMD : Flag<["-"], "MMD">, Group<M_Group>,
    HelpText<"Write a depfile containing user headers">;
...
def iquote : JoinedOrSeparate<["-"], "iquote">, Group<clang_i_Group>, Flags<[CC1Option]>,
  HelpText<"Add directory to QUOTE include search path">, MetaVarName<"<directory>">;
def isystem : JoinedOrSeparate<["-"], "isystem">, Group<clang_i_Group>,
  Flags<[CC1Option]>,
  HelpText<"Add directory to SYSTEM include search path">, MetaVarName<"<directory>">;

For comparison, the related MSVC options (also present in Options.td) are:

def _SLASH_headerUnitAngle : CLJoinedOrSeparate<"headerUnit:angle">;
def _SLASH_headerUnitQuote : CLJoinedOrSeparate<"headerUnit:quote">;

Should there be a "both" or "any" option so that the search considers all include paths as opposed to just user OR system?

iains added inline comments.Apr 25 2022, 12:00 PM
clang/docs/ClangCommandLineReference.rst
1187–1189

Are "user" and "system" the right terms to use here? Existing options aren't all that consistent. Some examples from Options.td:

def MD : Flag<["-"], "MD">, Group<M_Group>,
    HelpText<"Write a depfile containing user and system headers">;
def MMD : Flag<["-"], "MMD">, Group<M_Group>,
    HelpText<"Write a depfile containing user headers">;
...
def iquote : JoinedOrSeparate<["-"], "iquote">, Group<clang_i_Group>, Flags<[CC1Option]>,
  HelpText<"Add directory to QUOTE include search path">, MetaVarName<"<directory>">;
def isystem : JoinedOrSeparate<["-"], "isystem">, Group<clang_i_Group>,
  Flags<[CC1Option]>,
  HelpText<"Add directory to SYSTEM include search path">, MetaVarName<"<directory>">;

For comparison, the related MSVC options (also present in Options.td) are:

def _SLASH_headerUnitAngle : CLJoinedOrSeparate<"headerUnit:angle">;
def _SLASH_headerUnitQuote : CLJoinedOrSeparate<"headerUnit:quote">;

Should there be a "both" or "any" option so that the search considers all include paths as opposed to just user OR system?

1187–1189

At this time, the objective was to be command-line-option compatible with the existing GCC implementation. There is a goal to minimise difference in interfaces presented to the user (and build systems).

Given that the latter [GCC] is already "in the wild", if we wanted terminology-compatibility with MSVC, we could add aliases (to both), I suppose. I have no axe to grind here - if folks form a consensus that we could have better terminology, I'm happy to add the aliases etc.

There is also -fmodule-header ... which means "find the header at the path specified" which can either be 'absolute' (i.e. /xxxx on unix-y systems) or relative to CWD,

I'm not sure about 'both' or 'any' in the sense of how that would relate to the existing behaviour of header searches (the mechanisms for substituting HUs for textual headers assume that the searches are done with the same process as for the textual).

Hopefully, that expresses the intent of the current landed work - that is not to say that the story ends here, but that probably there are other features we want to complete before fine-tuning this.

tahonermann added inline comments.Apr 25 2022, 12:15 PM
clang/docs/ClangCommandLineReference.rst
1187–1189

Ah, I didn't realize gcc already implemented these options. I agree compatibility with it is the more important goal. Looks good to me.