[PATCH] D143587: [Docs] Multilib design

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

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.



================
Comment at: clang/docs/Multilib.rst:102
+There are two options permitted:
+#. Use only the *last* matching multilib variant.
+#. Use all matching variants, thereby layering them.
----------------
May be worth indicating that this is to support compatibility with existing clang multilib, we expect most implementations to use all matching variants.


================
Comment at: clang/docs/Multilib.rst:105
+
+This decision is hard-coded per ToolChain subclass.
+
----------------
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.


================
Comment at: clang/docs/Multilib.rst:122
+``-fno-exceptions`` multilib variant need only contain C++ libraries.
+
+Stability
----------------
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] 


================
Comment at: clang/docs/Multilib.rst:169
+    # considered a match.
+    flags: [V6]
+    # If a user invokes clang with -print-multi-lib then the arguments it
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143587/new/

https://reviews.llvm.org/D143587