This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Enable defining multilib flags verbatim
AbandonedPublic

Authored by michaelplatings on Mar 23 2023, 1:39 PM.

Details

Reviewers
phosek
Summary

Historically, a flag beginning - in multilib meant it was not compatible. So if you saw -fexceptions in multilib that meant exceptions were not allowed, which is confusingly the exact opposite of what it means when you see it on the command line.

This change allows doing the obvious thing of just specifying verbatim the flags needed by each multilib. The default of deriving print options from flags beginning '+' remains.

(Without this change, you could just use flags with a - prefix and everything except -print-multi-lib would work, so this change only needs to change how option printing works).

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 1:39 PM
michaelplatings requested review of this revision.Mar 23 2023, 1:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 1:39 PM

Approach looks good to me. Some suggestions, mostly around documenting the interface.

clang/include/clang/Driver/Multilib.h
63–64

I think it is worth adding some information from the commit message to the comment. In particular what the default behaviour is and what, if any, restrictions are there on the PrintOptionsList.

For example there is a comment in the assert in the constructor body that probably ought to be in the header

// If PrintOptions is something other than List then PrintOptionsList must be
// empty.

Are there any restrictions on the format of PrintOptionsList for it to print correctly? For example does each option need to be prefixed with -?

65

How would someone using makeMultilib() from MultilibBuilder use these parameters? If they can't do they need to? If so we may need something like a makeMultilib overload to supply the extra parameters.

clang/lib/Driver/Multilib.cpp
44

If there are any restrictions on the strings in PrintOptionsList could be worth assertions. Please ignore if there are no restrictions.

michaelplatings marked 3 inline comments as done.

Expand constructor comment as suggested by @peter.smith

clang/include/clang/Driver/Multilib.h
65

MultilibBuilder is constructed around the concept of using '+' and '-' to indicate which command line options are compatible and incompatible, so in that case you'd only want to use PlusFlags.

clang/lib/Driver/Multilib.cpp
44

There are restrictions - PrintOptionsList should only contain valid Clang options. However if you violate this then the only consequence is that the -print-multi-lib output will be equally invalid. Since there would be a lot of complexity involved in enforcing the restriction I think it's best to leave it unenforced.

michaelplatings updated this revision to Diff 518997.EditedMay 3 2023, 1:04 AM

Change code to allow defining multilib flags verbatim. I've removed the option to define flags and print options independently as discussed in https://discourse.llvm.org/t/rfc-multilib/67494

michaelplatings retitled this revision from [Driver] Enable defining multilib print options independently of flags to [Driver] Enable defining multilib flags verbatim.May 3 2023, 1:10 AM
michaelplatings edited the summary of this revision. (Show Details)

Re-run arc diff with clang-format in PATH

michaelplatings edited the summary of this revision. (Show Details)May 3 2023, 1:22 AM
michaelplatings edited the summary of this revision. (Show Details)May 3 2023, 3:03 AM

I apologize about the delayed response. I had time to think about this a bit more and it's not really clear to me if we need to preserve the "plus/minus" flag syntax. Looking through the history, it seems that it was introduced in D2538 without any discussion but I always considered it to be confusing and potentially error-prone. Rather than supporting two different syntaxes, which introduces more complexity, I'd prefer having just one. I can of think of two approaches:

  1. Have two separate lists of include and exclude flags.
  2. Store flags as a tuple (or a struct?) of string and a tag (that is include or exclude).

Do you have any preference between one these two approaches? Can you think of alternatives? It's going to take a bit of effort to migrate all the existing use cases of the "plus/minus" syntax, but I hope it should be mostly mechanical. I'd be happy to help with the migration once we settle on an API.

Funnily enough I had the same thought coming to work this morning that we don't need 2 syntaxes. I had an idea that we could only print flags beginning "-" but will consider the other options you suggested as well. Thanks.

michaelplatings abandoned this revision.May 25 2023, 6:17 AM

Abandoning this in favour of D151437 & D151438.