[PATCH] D72756: [LLD][ELF] Add support for INPUT_SECTION_FLAGS

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/LinkerScript.cpp:434
           pat.excludedFilePat.match(filename) ||
-          !pat.sectionPat.match(sec->name))
+          !pat.sectionPat.match(sec->name) ||
+          (sec->flags & cmd->withFlags) != cmd->withFlags ||
----------------
This may need a rebase if D72775 lands first.


================
Comment at: lld/ELF/LinkerScript.h:159
+  InputSectionDescription(StringRef filePattern, uint64_t withFlags = 0,
+                          uint64_t withoutFlags = 0)
+      : BaseCommand(InputSectionKind), filePat(filePattern),
----------------
peter.smith wrote:
> grimar wrote:
> > I wonder, can we have something like `flagsMask` here and everywhere instead of two variables?
> I couldn't think of a way of doing it with just one flagsMask as for each flag bit there are 3 states (don't care, required set, required not-set) and with a single mask we've only got 2. An alternative to the above is to use flagsMask and withMask. Both with and without would add to flagsMask, but only with would add to withMask. Then to check we have just. 
> ```
> (sec->flags | flagsMask) == withMask; 
> ```
> That saves a comparison at, I guess a slight cost to readability, but we'd still need two values. Happy to change to that if you'd prefer it. Open to alternative suggestions about how to represent it.
The transformation is `(sec->flag | dont-care) = (withMask | dont-care)`, i.e.

`(sec->flags | ~(withMask | withoutMask)) == (withMask | ~(withMask | withoutMask))`

It harms readability and unlikely improves performance.


================
Comment at: lld/ELF/ScriptParser.cpp:1164
+    .Case("SHF_MIPS_STRING", 0x80000000)
+    .Case("SHF_ARM_PURECODE", 0x2000000)
+    .Default(None);
----------------
peter.smith wrote:
> grimar wrote:
> > This is perhaps not a best idea how to handle flags?
> > I guess we should not accept MIPS flags for non-mips targets and so on?
> > (I see that we probably do not know the target plafrorm at this point, I think).
> > 
> > I wonder, can we just support a generic set for start? Will it be enough?
> > I.e. write/alloc/exec flags (+something else)?
> I'd be happy with that, although I'd prefer we supported integers in that case as then if we'd not included someone's favourite flag they can still use it. The GNU linker supports:
> 
> ```
>   { "SHF_WRITE", SHF_WRITE },
>   { "SHF_ALLOC", SHF_ALLOC },
>   { "SHF_EXECINSTR", SHF_EXECINSTR },
>   { "SHF_MERGE", SHF_MERGE },
>   { "SHF_STRINGS", SHF_STRINGS },
>   { "SHF_INFO_LINK", SHF_INFO_LINK},
>   { "SHF_LINK_ORDER", SHF_LINK_ORDER},
>   { "SHF_OS_NONCONFORMING", SHF_OS_NONCONFORMING},
>   { "SHF_GROUP", SHF_GROUP },
>   { "SHF_TLS", SHF_TLS },
>   { "SHF_MASKOS", SHF_MASKOS },
>   { "SHF_EXCLUDE", SHF_EXCLUDE },
> ```
> Which is pretty much just the generic ones. I know SHF_ARM_PURECODE would be really useful, but that could be represented as an integer.
> 
We can do something like `tools/llvm-readobj/ELFDumper.cpp`:

```
#define ENUM_ENT(enum) { #enum, ELF::enum }

ENUM_ENT(SHF_WRITE)
ENUM_ENT(SHF_ALLOC)
```

I agree that the generic ones plus SHF_ARM_PURECODE should be sufficient. 


================
Comment at: lld/test/ELF/input-section-flags-diag1.test:2
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux /dev/null -o %t.o
+# RUN: not ld.lld -shared %t.o -o %t --script %s 2>&1 | FileCheck -strict-whitespace %s
----------------
Nit: `-unknown-linux` -> ``(This is generic, not Linux specific)


================
Comment at: lld/test/ELF/input-section-flags-diag1.test:3
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux /dev/null -o %t.o
+# RUN: not ld.lld -shared %t.o -o %t --script %s 2>&1 | FileCheck -strict-whitespace %s
+
----------------
%t -> /dev/null


================
Comment at: lld/test/ELF/input-section-flags-diag2.test:3
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux /dev/null -o %t.o
+# RUN: not ld.lld -shared %t.o -o %t --script %s 2>&1 | FileCheck -strict-whitespace %s
+
----------------
%t -> /dev/null


================
Comment at: lld/test/ELF/input-section-flags.s:4
+
+# Test the INPUT_SECTION_FLAGS feature. It prefixes an input section list and
+# restricts matches to sections that have the required flags and do not have
----------------
`##`


================
Comment at: lld/test/ELF/input-section-flags.s:11
+# RUN: echo "SECTIONS { \
+# RUN: .outsec1 : { INPUT_SECTION_FLAGS(SHF_ALLOC & !SHF_EXECINSTR & !SHF_WRITE & !SHF_MERGE) *(.sec*) }\
+# RUN: .outsec2 : { INPUT_SECTION_FLAGS(SHF_ALLOC & SHF_EXECINSTR & !SHF_WRITE & !SHF_MERGE) *(.sec* .text) } \
----------------
Nit: Indent continuation lines.


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

https://reviews.llvm.org/D72756