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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/ELF/ScriptParser.cpp:1164
+    .Case("SHF_MIPS_STRING", 0x80000000)
+    .Case("SHF_ARM_PURECODE", 0x2000000)
+    .Default(None);
----------------
peter.smith wrote:
> 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.
Accept an integer is a nice extension.

The loop can be simplified as:

```lang=cpp
  while (!errorCount()) {
    StringRef tok = unquote(next());
    bool without = tok.consume_front("!");
    if (llvm::Optional<uint64_t> flag = parseFlag(tok)) {
      if (without)
        withoutFlags |= *flag;
      else
        withFlags |= *flag;
    } else {
      setError("unrecognised flag: " + tok);
    }
    if (consume(")"))
      break;
    if (!consume("&")) {
      next();
      setError("expected & or )");
    }
  }
```


================
Comment at: lld/ELF/ScriptParser.cpp:864
+    } else if (tok == "INPUT_SECTION_FLAGS") {
+        std::tie(withFlags, withoutFlags) = readInputSectionFlags();
     } else if (peek() == "(") {
----------------
Mis-indented.


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

https://reviews.llvm.org/D72756