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

Thu Jan 16 09:25:39 PST 2020

peter.smith marked 22 inline comments as done.
peter.smith added a comment.

Thanks for the comments, I'll upload a new patch shortly.

================
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 ||
----------------
MaskRay wrote:
> This may need a rebase if D72775 lands first.
I've rebased as D72775 has landed.

================
Comment at: lld/ELF/LinkerScript.h:159
+  InputSectionDescription(StringRef filePattern, uint64_t withFlags = 0,
+                          uint64_t withoutFlags = 0)
+      : BaseCommand(InputSectionKind), filePat(filePattern),
----------------
MaskRay wrote:
> 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.
Thanks, I'll leave it as it is for now.

================
Comment at: lld/ELF/ScriptParser.cpp:1164
+    .Case("SHF_MIPS_STRING", 0x80000000)
+    .Case("SHF_ARM_PURECODE", 0x2000000)
+    .Default(None);
----------------
MaskRay wrote:
> 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. 
I'll implement using ENUM_ENT and will restrict to generic + SHF_ARM_PURECODE. I don't think that it is that important to filter the available flags by Target as we are mapping unique name to value and we're not mapping back from non-unique value to name.

I've kept the ability to accept an int for now. Happy to remove if you'd prefer, given it reuses an existing function it is not a lot of extra effort to include.

================
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
----------------
MaskRay wrote:
> Nit: `-unknown-linux` -> ``(This is generic, not Linux specific)
will fix.

================
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
+
----------------
MaskRay wrote:
> %t -> /dev/null
will fix

================
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
+
----------------
MaskRay wrote:
> %t -> /dev/null
Will fix.

================
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
----------------
MaskRay wrote:
> `##`
will fix

================
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) } \
----------------
MaskRay wrote:
> Nit: Indent continuation lines.
Will fix.

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

https://reviews.llvm.org/D72756