Details
- Reviewers
peter.smith phosek MaskRay - Commits
- rGa5aeba737694: [Docs] Multilib design
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
86–89 | Would it be possible to give more details on how the toolchain makes this choice? | |
97 | Would it be worth to also add a flag to activate/deactivate the multilib feature for the duration that it is experimental? |
clang/docs/Multilib.rst | ||
---|---|---|
89 | What would be the effect of layering? Does this mean multiple choices for --sysroot, -isystem and -L` will be used? |
clang/docs/Multilib.rst | ||
---|---|---|
86–89 | Added below: "This decision is hard-coded per ToolChain subclass" | |
89 | Added a section below on multilib layering | |
97 | 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:
| |
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. |
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" |
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!) 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. |
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? |
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. |
- Make "experimental" more obvious.
- Demonstrate using a Clang-generated flag directly in Flags
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? |
clang/docs/Multilib.rst | ||
---|---|---|
242 | Add quotes |
LGTM
clang/docs/Multilib.rst | ||
---|---|---|
232 | Use backticks to render the flag name as code, same for the other instances in this paragraph. |
s/clang/Clang/