[clang][driver] Restore the original help text for `-I`

[awarzynski]

Summary

The help text for -I was recently expanded in this commit:

• 8dd4e3ceb804a58bcf25e6856fc6fde5e1995a66

The updated version:

• describes the internal implementation in Clang, and
• is inconsistently long when compared to other options printed with clang -help.

We are now in the process of adding support for -I in Flang and this
expanded description is invalid as far as Flang is concerned (i.e. it
does not reflect Flang's internal implementation). See this review:

• https://reviews.llvm.org/D93453

This patch reverts the original change in Options.td. This way the
description of -I remains generic and valid for both Clang and Flang.

Clang's internal documentation for -I is available in
ClangCommandLineReference.rst (it's identical to what's being removed
here, so no information is lost).

[richard.barton.arm]

The rationale for this revert looks good to me.

I don't see the rationale behind the original change as it was committed without review. If @andrei99 gets back to us then we can consider reinstating it, perhaps in an adjusted form.

[andreil99]

I'm fine with having this text in the ClangCommandLineReference.rst only, and removing it from clang -help.

However, reverting the patch would not do, as ClangCommandLineReference.rst is auto-generated from clang/include/clang/Driver/Options.td. Please feel free to propose a patch for this.

The rationale for the change was multiple requests from our users, since this is important for them and is what they have to discover by experiments.

[awarzynski]

In D94169#2484968, @andreil99 wrote:

I'm fine with having this text in the ClangCommandLineReference.rst only, and removing it from clang -help.

However, reverting the patch would not do, as ClangCommandLineReference.rst is auto-generated from clang/include/clang/Driver/Options.td. Please feel free to propose a patch for this.

The rationale for the change was multiple requests from our users, since this is important for them and is what they have to discover by experiments.

Thank you for providing the context, that's much appreciated. As the original patch already contained changes in ClangCommandLineReference.rst , I assumed that it was completely separate from Options.td. My bad for not double-checking.

To the best of my knowledge, there is no mechanism to tweak the help message for an option (i.e. what's specified in Options.td cannot be tweaked elsewhere). So the only alternative for Flang would be to introduce a completely new option to represent -I, e.g.:

def I_for_Flang JoinedOrSeparate<["-"], "I">, Group<I_Group>, Flags<[FC1Option, FlangOption]>, MetaVarName<"<dir>">,
    HelpText<"Add directory to include search path>;

However, as Flang and Clang are meant to share libclangDriver [1], and Options.td is part of libclangDriver, I feel that these drivers should also share as many option definitions as possible. To me OPT_I is a good example of an option that should be shared.

I'm keen to resolve this in a way that's satisfactory for everyone. Personally I feel that adding def I_for_Flang would be detrimental to libclangDriver. Bearing in mind that the original change was introduced without a review, I would appreciate if more people could chime in. Any thoughts @richard.barton.arm ?

[1] https://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html

[andreil99]

Post commit review is a normal practice in the LLVM community. This is not an excuse to revert somebody's patch per se, unless there are other serious reasons for the revert. The text does not describe “clang internals”, it describes the semantic of that flag with respect to other flags specifically to clang (I’m sure the text could be improved and open to suggestions).

At the time when the original patch was done this td file was not shared with flang and such plans weren’t announced, it was quite the opposite, one of the flang driver RFCs specifically mentioned a separate *.td file, if memory serves me well. And I do apologize if I missed something, I didn’t follow flang discussions closely back in July/August of the last year.

Now since you have started sharing, we need to find a good way to have front-end specifics in the documentation, which happens to be auto-generated from a shared td file. Stripping it down to only matching subset of things between multiple different front-ends would hurt documentation quality. Let’s discuss here of what could be done about that, or feel free to move the discussion somewhere else.

[richard.barton.arm]

Could we tweak the wording to clarify that it is Clang-specific?

[sscalpone]

@awarzynski, perhaps I am being too dramatic by wondering if we are compromising support for one or the other language in order to support a common definition. In the case of -I, there are language-specific semantics that are important and interesting, such as the relationship in clang between -I and -isystem, and the relationship in flang between -I and -module. Maybe the right answer is to allow the definition of duplicate options that are differentiated by language?

+1 @andreil99

[awarzynski]

Thank you all for your input. Let me summarize.

SUGGESTION 1
Tweak the wording to clarify that it is Clang-specific. That's a good compromise IMO. It highlights that the described semantics apply to Clang only. And I'm not aware of any other way to achieve this right now (i.e. without intrusive refactoring).

SUGGESTION 2:
The description of -I in -help and ClangCommandLineReference.rst are made independent of each other (so that -help can be tweaked to suit both Clang and Flang).

ClangCommandLineReference.rst is basically generated from Options.td, so any modification to Options.td will affect the user-facing docs.

From docs/CMakeLists.txt:

gen_rst_file_from_td(ClangCommandLineReference.rst -gen-opt-docs ../include/clang/Driver/ClangOptionDocs.td docs-clang-html)

From ClangOptionDocs.td:

include "Options.td"

I don't see any straightforward solution here that would allow different descriptions for -I in -help and in the user-facing docs. Not without completely refactoring how ClangCommandLineReference.rst is produced.

SUGGESTION 3
Ideally, we'd have one help message for -I for Clang and another one for Flang (in Options.td). Unfortunately, AFAIK, there is no easy way to tweak the output of -help. The current API is quite inflexible in this respect. This was brought-up at some point last year [1][2]. From the limited feedback I'm guessing that that would also require a major refactor.

SUGGESTION 4
We add a new definition for -I for Flang, e.g. def I_for_flang.

This solves the problem, but adds yet another option definition. Note that libclangDriver creates only one global list of all options:

• https://github.com/llvm/llvm-project/blob/80dee7965dffdfb866afa9d74f3a4a97453708b2/clang/lib/Driver/DriverOptions.cpp#L42-L55
• https://github.com/llvm/llvm-project/blob/80dee7965dffdfb866afa9d74f3a4a97453708b2/clang/include/clang/Driver/Options.h#L43-L52

This means that both Flang and Clang will have access to OPT_I and OPT_I_for_flang (C++ options generated from Options.td). These can be hidden from the user, but will still be available. Also, this would mean the massive table of libclangDriver options keeps growing.

Is that how we should proceed moving forward when option names are identical for Clang and Flang, but the semantics are slightly different?

[1] https://lists.llvm.org/pipermail/llvm-dev/2020-July/143745.html
[2] https://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html

Few final points
I would really appreciate a compromise that does not require any major refactoring in Clang at this stage.
We are ready to add -Ito Flang and we have a bunch of patches almost ready that depend on it.
I appreciate that this is just one option, but we are likely to experience more inconsistencies like this in the near future.

Also, for reference, output from gcc and gfortran:

-I <dir>                    Add <dir> to the end of the main include path.

[awarzynski]

Implement SUGGESTION 1

I haven't updated the patch description yet - I'd rather keep it as is while the actual direction for this patch is being discussed.

Also, I regenerated ClangCommandLineReference.rst and discovered that many options are out-of-sync with Options.td. In order not to digress, I've only updated the description for -I.

@andreil99 @richard.barton.arm @sscalpone , any thoughts? IMO only *SUGGESTION 4* is a feasible alternative at this stage. But for that approach I'd like to consult cfe-dev first (to clarify the direction for similar options in the future).

[richard.barton.arm]

I think the right long-term solution is going to be SUGGESTION 2, or a variant of that. We have implemented this feature downstream in Arm Compiler for Linux, i.e. a separate, normally longer and more detailed, Doc string for the CommandLineReference.rst file from the --help string. Such a mechanism could perhaps be extended to add Clang- and/or Flang- specific doc strings or additional doc strings. That's a big piece of work, but very worthy IMO.

To unblock development today I would support Andrzej's SUGGESTION 1. Perhaps saying "For C++ input ..." to distinguish the Clang-only behaviour for now.

In writing the above I double checked the Options.td to see if anything upstream exists today. There is a field DocBrief that appears to be preferred over HelpText in the ClangCommandLineReference.rst. Perhaps the new, more detailed but C++ specific message could be a DocBrief field, so for the ClangCommandLineReference.rst only and we can revert to the original shorter HelpText that applies for both C++ and Fortran? Perhaps that would actually be a better short-term compromise in that it meets @andreil99 's needs as well as Flangs immediate needs?

[awarzynski]

Use DocBrief, as suggested by @richard.barton.arm.

[andreil99]

Thanks for suggesting DocBrief, Richard!

Looks good to me with a nit.

clang/include/clang/Driver/Options.td

654–660

How about Add directory to the end of the list of include search paths or something similar? There is an order in which the directories are used.

[awarzynski]

Thank you for the reviewing @andreil99 !

clang/include/clang/Driver/Options.td

654–660

I'll update this before merging.

[awarzynski]

I have now merged this into main. I updated the commit message based on the discussion here, but kept the original SUMMARY above (just in case people are confused by the inconsistency).

clang/docs/ClangCommandLineReference.rst

 
 Include path management
 -----------------------
 
 Flags controlling how ``#include``\s are resolved to files.
 
 .. option:: -I<dir>, --include-directory <arg>, --include-directory=<arg>
 
-Add directory to include search path. If there are multiple -I options, these directories are searched in the order they are given before the standard system directories are searched. If the same directory is in the SYSTEM include search paths, for example if also specified with -isystem, the -I option will be ignored
+Add directory to include search path. For C++ input, if
+there are multiple -I options, these directories are searched
+in the order they are given before the standard system directories
+are searched. If the same directory is in the SYSTEM include search
+paths, for example if also specified with -isystem, the -I option
+will be ignored
 
 .. option:: -I-, --include-barrier
 
 Restrict all prior -I flags to double-quoted inclusion and remove current directory from include path
 
 .. option:: --cuda-path-ignore-env
 
 Ignore environment variables to detect CUDA installation

clang/include/clang/Driver/Options.td

 def H : Flag<["-"], "H">, Flags<[CC1Option]>, Group<Preprocessor_Group>,
     HelpText<"Show header includes and nesting depth">,
     MarshallingInfoFlag<"DependencyOutputOpts.ShowHeaderIncludes">;
 def I_ : Flag<["-"], "I-">, Group<I_Group>,
     HelpText<"Restrict all prior -I flags to double-quoted inclusion and "
              "remove current directory from include path">;
 def I : JoinedOrSeparate<["-"], "I">, Group<I_Group>,
     Flags<[CC1Option,CC1AsOption]>, MetaVarName<"<dir>">,
-    HelpText<"Add directory to include search path. If there are multiple -I "
-             "options, these directories are searched in the order they are "
-             "given before the standard system directories are searched. "
-             "If the same directory is in the SYSTEM include search paths, for "
-             "example if also specified with -isystem, the -I option will be "
-             "ignored">;
+    HelpText<"Add directory to the end of the list of include search paths">,
+    DocBrief<[{Add directory to include search path. For C++ inputs, if
+there are multiple -I options, these directories are searched
+in the order they are given before the standard system directories
+are searched. If the same directory is in the SYSTEM include search
+paths, for example if also specified with -isystem, the -I option
+will be ignored}]>;
 def L : JoinedOrSeparate<["-"], "L">, Flags<[RenderJoined]>, Group<Link_Group>,
     MetaVarName<"<dir>">, HelpText<"Add directory to library search path">;
 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 M : Flag<["-"], "M">, Group<M_Group>,
     HelpText<"Like -MD, but also implies -E and writes to stdout by default">;