This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by awarzynski on Jan 6 2021, 5:00 AM.

Details

Summary

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

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:

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).

Diff Detail

Event Timeline

awarzynski created this revision.Jan 6 2021, 5:00 AM
awarzynski requested review of this revision.Jan 6 2021, 5:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2021, 5:00 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
awarzynski added a project: Restricted Project.

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.

This revision is now accepted and ready to land.Jan 7 2021, 9:08 AM
andreil99 requested changes to this revision.Jan 7 2021, 11:00 AM

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.

This revision now requires changes to proceed.Jan 7 2021, 11:00 AM

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

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.

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

@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

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:

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 updated this revision to Diff 315765.EditedJan 11 2021, 5:17 AM

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).

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?

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

andreil99 accepted this revision.Jan 12 2021, 12:21 PM

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.

This revision is now accepted and ready to land.Jan 12 2021, 12:21 PM

Thank you for the reviewing @andreil99 !

clang/include/clang/Driver/Options.td
654–660

I'll update this before merging.

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).