This is an archive of the discontinued LLVM Phabricator instance.

[Docs] Multilib design
ClosedPublic

Authored by michaelplatings on Feb 8 2023, 7:29 AM.

Diff Detail

Unit TestsFailed

Event Timeline

michaelplatings created this revision.Feb 8 2023, 7:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2023, 7:29 AM
Herald added a subscriber: arphaman. · View Herald Transcript
michaelplatings requested review of this revision.Feb 8 2023, 7:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2023, 7:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for the design docs. At a high-level this gives a good summary on what you intend the multilib feature to do. Couple of suggestions.

clang/docs/Multilib.rst
87–90

Would it be possible to give more details on how the toolchain makes this choice?

98

Would it be worth to also add a flag to activate/deactivate the multilib feature for the duration that it is experimental?
IIUC, a similar mechanism was used before switching to a new pass manager.

amilendra added inline comments.Feb 9 2023, 2:59 AM
clang/docs/Multilib.rst
90

What would be the effect of layering? Does this mean multiple choices for --sysroot, -isystem and -L` will be used?

Made changes requested by @amilendra

michaelplatings marked 3 inline comments as done.Feb 9 2023, 3:40 AM
michaelplatings added inline comments.
clang/docs/Multilib.rst
87–90

Added below: "This decision is hard-coded per ToolChain subclass"

90

Added a section below on multilib layering

98

I think no because the feature is implicitly disabled by a lack of multilib.yaml. I added some text explaining what's necessary to enable the feature which hopefully makes that clearer.

Thanks very much for writing this. Will be worth updating the RFC with a link as I think that this is an approachable place to comment on the format and selection model without the implementation detail. I'm happy with what has been written so far. My suggestions are mostly additions rather than modifications.

Could be worth adding a design principles section at the end, this may help someone looking back rationalise the choices made. For example I think that it is an important decision to expand the command-line options in clang prior to multi-lib selection to avoid another implementation of the complex architecture feature selection code in multilib.

May be worth a quick description of how we might model some awkward cases like position independent and non-position independent code. This is analagous to soft and softfp.

Position independence is an awkward case to model, which may be worth showing as an example.
* if my incoming flags include position independent code [PIC] then I need to select the [PIC] libraries, which implies that we want a [PIC] flag.
* if my incoming flags do not include position independent code then I can use [PIC] libraries, but I would prefer to use not PIC if it were available.
If we did have separate PIC and non PIC choices then I think this would need two features [PIC] and [non-PIC] with incoming non-position independent code having both [PIC, non-PIC] so it could match both, but position indendent code would only have [PIC]. Presumably we'd need the [non-PIC] variant sorted after.
clang/docs/Multilib.rst
103

May be worth indicating that this is to support compatibility with existing clang multilib, we expect most implementations to use all matching variants.

106

Considering the BareMetal ToolChain is used by RISC-V, Arm and AArch64 is there scope for a different choice per toolchain. I guess that we could put a predicate function for the ToolChain that could change due to architecture.

123

Although it can be worked out by looking at the example, which I think is your intention from the comment. It may be worth a section on Syntax that extracts some of these.

Syntax of multilib.yaml
===============

Some suggestions/questions:

  • Does the file have to contain both variants: and flagmap: are either of them optional?
  • Do variants have to come before flagmap, or is order not important.
  • Will be worth highlighting that the order of the variants may be significant depending on the toolchain.
  • Each variant is made up of dir: (path added to sysroot?), flags: [comma separated list of flags] and printArgs: [comma separated list of command line options for -print-multi-lib and -print-multi-directory, but not important for multilib selection]
  • Each flagmap is made up of a POSIX extended syntax regex: that matches a flag, and (one of?) matchFlags/noMatchFlags which either adds or removes respecively a [comma separated list of flags]
170

Although very Arm specific. I'd use [V6M] here as ArmV6 (pre-cortex so no explicit profile) is very different to ArmV6-m. Also in regex section below.

michaelplatings marked 7 inline comments as done.

Incorporate suggestions from @peter.smith

Could be worth adding a design principles section at the end

That was a very helpful suggestion, thank you. Writing them out helped me examine the compatibility story more carefully. Consequently I added a requirement for a "minimumClangVersion" property, although I haven't implemented that in the other patches yet.

May be worth a quick description of how we might model some awkward cases

Not done yet. This certainly can be modelled but I agree an example would be helpful to provide.

clang/docs/Multilib.rst
106

For the BareMetal ToolChain the only one RISC-V multilibs can ever match a given set of command line options so this isn't a problem in practice.

123

I opted to keep the spec, such as it is, in the example, but I've expanded it to answer your questions, unless it's implicit in it being YAML (e.g. lists can be comma separated but there are other ways to express them in YAML).

Thanks for the update, one nit in the language, otherwise looks good to me. It is a good reflection of the implementation.

clang/docs/Multilib.rst
251

"The API need only multilib selection" doesn't look right. Perhaps "The API can do multilib selection"

peter.smith added inline comments.Feb 10 2023, 8:04 AM
clang/docs/Multilib.rst
126

Although implicit in the mechanism, is it worth highlighting that layered multib.yaml authors will need to make sure that the includes and the libraries in the layer need to be complete enough to mask any incompatibilities?

For example if in the -fno-exceptions case there exists a library libX.a that is affected by -fno-exceptions but is present in the "Core directory" but not the layered no-exceptions directory then the exceptions libX.a will be selected.

/fp/libX.a (should have a noexcept variant in noexcept but multilib implementer gets it wrong!)
/fp/libY.a
/fp/lib/noexecpt/libY.a

execeptions is perhaps not the best example here as including a file with exceptions when the intention is -fno-exceptions is not optimal, but it will work. If there is layering for floating point calling conventions then a mix could result in a broken program.

Tweak multilib.yaml spec slightly

MaskRay added inline comments.
clang/docs/Multilib.rst
42

s/clang/Clang/

43

Where is MultilibBuilder?

44

lib/Driver/ToolChains/Gnu.cpp

163

Generally the comments in this example feel verbose. Consider changing some sentences with briefer but equivalent wording.

For example, "It is required to be present." is repeated several times. It can be combined with a previous sentence like "This is required and ..." or "This required key defines ..."

230

I think Example of noMatchFlags - is verbose.

238

What does "API" refer to? A function defined in llvm-project/clang/lib?

michaelplatings marked 8 inline comments as done.

Rebase and apply changes requested by @MaskRay and @peter.smith

clang/docs/Multilib.rst
238

Changed "API" to "interface" and reworded to hopefully make it clearer.

PrintArgs -> PrintOptions

  • Make "experimental" more obvious.
  • Demonstrate using a Clang-generated flag directly in Flags

flags -> tags

peter.smith accepted this revision.Mar 13 2023, 9:27 AM

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions. I've left a suggestion that can be applied prior to commit if you want to take it up.

clang/docs/Multilib.rst
57

Can we add a forward reference to the Stable Interface design principle, or perhaps move it here so that it is more obvious?

This revision is now accepted and ready to land.Mar 13 2023, 9:27 AM

Reference Multilib.rst from comment in Multilib.h

MaskRay accepted this revision.Mar 14 2023, 3:06 PM
MaskRay added inline comments.
clang/docs/Multilib.rst
242

Add quotes

michaelplatings marked an inline comment as done.

Add quotes

Update docs to reflect recent changes

FlagMap->Mappings

phosek accepted this revision.Jun 9 2023, 1:11 AM

LGTM

clang/docs/Multilib.rst
232

Use backticks to render the flag name as code, same for the other instances in this paragraph.

This revision was landed with ongoing or failed builds.Jun 13 2023, 10:47 PM
This revision was automatically updated to reflect the committed changes.
michaelplatings marked an inline comment as done.Jun 13 2023, 10:50 PM