[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 comments, I'll make an update tomorrow. I'll plan to:

- Try the flagsMask + withMask to see if it looks any better.
- Limit the recognised flags to what BFD accepts, I'd like to continue to recognise integers.
- Look at the error reporting in the parser.
- Update the tests.



================
Comment at: lld/ELF/LinkerScript.h:159
+  InputSectionDescription(StringRef filePattern, uint64_t withFlags = 0,
+                          uint64_t withoutFlags = 0)
+      : BaseCommand(InputSectionKind), filePat(filePattern),
----------------
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.


================
Comment at: lld/ELF/ScriptParser.cpp:1136
+  if (llvm::Optional<uint64_t> asInt = parseInt(tok))
+    return asInt;
+  return StringSwitch<llvm::Optional<uint64_t>> (tok)
----------------
grimar wrote:
> Are integers used in the wild? I wonder if we can stick to supporting named flags only,
> it should be easier to read scripts I think.
I've not seen them used, but it does offer an instant workaround if a new flag is added in a future release, or a more limited set of flags is chosen to be accepted.


================
Comment at: lld/ELF/ScriptParser.cpp:1164
+    .Case("SHF_MIPS_STRING", 0x80000000)
+    .Case("SHF_ARM_PURECODE", 0x2000000)
+    .Default(None);
----------------
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.



================
Comment at: lld/ELF/ScriptParser.cpp:1186
+      if (consume("&") || consume(")"))
+        setError("expected SectionFlag or !SectionFlag");
+      StringRef tok = unquote(next());
----------------
grimar wrote:
> This probably is not correct way. What if we have a different unexpected token here, like "+"?
> It seems you should be able to remove this `setError`.
Agreed I'll rethink and see if I can come up with something better.


================
Comment at: lld/ELF/ScriptParser.cpp:1191
+      if (!flag)
+        setError("unrecognised flag");
+      else {
----------------
grimar wrote:
> You need to print the value I think.
Agreed, thanks for the suggestion


================
Comment at: lld/test/ELF/input-section-flags.s:97
+
+## SHF_ALLOC
+ .section .sec1, "a", @progbits
----------------
grimar wrote:
> I'd suggest to rename sections instead.
> To: `.sec.a`, `.sec.ax`, etc.
> 
> Because such comments are not really useful by themselves probably?
OK, I'll update.


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

https://reviews.llvm.org/D72756