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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

First round of comments from me.
I mostly was focused on "is it possible to simplify the implementation for start?" question.



================
Comment at: lld/ELF/LinkerScript.h:159
+  InputSectionDescription(StringRef filePattern, uint64_t withFlags = 0,
+                          uint64_t withoutFlags = 0)
+      : BaseCommand(InputSectionKind), filePat(filePattern),
----------------
I wonder, can we have something like `flagsMask` here and everywhere instead of two variables?


================
Comment at: lld/ELF/ScriptParser.cpp:1136
+  if (llvm::Optional<uint64_t> asInt = parseInt(tok))
+    return asInt;
+  return StringSwitch<llvm::Optional<uint64_t>> (tok)
----------------
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.


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


================
Comment at: lld/ELF/ScriptParser.cpp:1186
+      if (consume("&") || consume(")"))
+        setError("expected SectionFlag or !SectionFlag");
+      StringRef tok = unquote(next());
----------------
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`.


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


================
Comment at: lld/test/ELF/input-section-flags.s:97
+
+## SHF_ALLOC
+ .section .sec1, "a", @progbits
----------------
I'd suggest to rename sections instead.
To: `.sec.a`, `.sec.ax`, etc.

Because such comments are not really useful by themselves probably?


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

https://reviews.llvm.org/D72756