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

[Peter Smith via Phabricator via llvm-commits]

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

Thanks for the review. I'll upload another patch that I hope to have addressed all the comments.



================
Comment at: lld/ELF/ScriptParser.cpp:1164
+    .Case("SHF_MIPS_STRING", 0x80000000)
+    .Case("SHF_ARM_PURECODE", 0x2000000)
+    .Default(None);
----------------
MaskRay wrote:
> 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 )");
>     }
>   }
> ```
Thanks for the suggestion, I've applied.


================
Comment at: lld/ELF/ScriptParser.cpp:709
+    StringRef tok = next();
+    if (tok == "INPUT_SECTION_FLAGS") {
+      std::tie(withFlags, withoutFlags) = readInputSectionFlags();
----------------
grimar wrote:
> I think more natural way is to use `consume`:
> 
> ```
> if (consume("INPUT_SECTION_FLAGS"))
>   std::tie(withFlags, withoutFlags) = readInputSectionFlags();
> 
> StringRef filePattern = next();
> ...
> ```
Thanks for the suggestion, have applied and also in readOverlayDescription.


================
Comment at: lld/ELF/ScriptParser.cpp:863
       readInclude();
+    } else if (tok == "INPUT_SECTION_FLAGS") {
+        std::tie(withFlags, withoutFlags) = readInputSectionFlags();
----------------
grimar wrote:
> We parse "KEEP" in `readInputSectionDescription`, and you handle "INPUT_SECTION_FLAGS" for it right there.
> Seems you can remove handling of "INPUT_SECTION_FLAGS" from here and move it to `readInputSectionDescription` too
> (for the no-KEEP case).
> 
> This should allow avoid having `withFlags = withoutFlags = 0;` code (and variables itself) around.
On my first attempt I had the logic in readInputSectionDescription. The problem I found was with the no section description case matched in:
```
else {
      // We have a file name and no input sections description. It is not a
      // commonly used syntax, but still acceptable. In that case, all sections
      // from the file will be included.
}
```
As INPUT_SECTION_DESCRIPTION always has parentheses it is matched by
```
else if (peek() == "(") 
```
So if I write: 
``` 
{ INPUT_SECTION_DESCRIPTION(some flag list) filespec } 
``` 
it matches the else if (peek() == "("). Unfortunately the above is not easily distinguishable from:
```
{ INPUT_SECTION_DESCRIPTION(some flag list) filespec(inputsectiondescription) }
```
I couldn't think of an easy way of distinguishing between these two cases at this level. I'm open to suggestions as I'm not keen on having to clear the state.


================
Comment at: lld/ELF/ScriptParser.cpp:1146
+  };
+#undef ENUM_ENT
+  if (llvm::Optional<uint64_t> asInt = parseInt(tok))
----------------
grimar wrote:
> grimar wrote:
> > May be use `llvm::StringSwitch` instead?
> To clarify: `StringSwitch` looks probably much simpler than a combination of `EnumEntry<unsigned>` array + `llvm::find_if`.
> So it seems could be:
> 
> ```
>   return StringSwitch<llvm::Optional<uint64_t>> (tok)
>     .Case("SHF_WRITE", ELF::SHF_WRITE)
>  ...
> ```
> 
> or
> 
> ```
>   return StringSwitch<llvm::Optional<uint64_t>> (tok)
>     .Case(ENUM_ENT(SHF_WRITE))
>  ...
> ```
> (If we want to use defines. I am not sure in that, given that the number of flags is not so large now,
> but have no strong objections)
> 
> Both ways still look better probably?
I've rewritten using StringSwitch, thanks for the suggestion, I think it looks simpler.


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

https://reviews.llvm.org/D72756