[PATCH] D91180: [ELF] Support multiple SORT in an input section description

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/ScriptParser.cpp:659
+    // SORT(.bar))) where we should break the loop.
+    while (!errorCount() && peek() != ")" && peek2() != "(")
       SectionMatcher.addPattern(unquote(next()));
----------------
grimar wrote:
> I'd reformat the comment to have the sample on a single line, it is hard to read currently.
> 
> ```
>     // peek2() is used to detect "EXCLUDE_FILE(" or "SORT("
>     // (e.g. *(.foo SORT(.bar)) ) where we should break the loop.
> ```
> 
> Also, our current grammar is LL(2), but we tried to use LL(1) where was possible.
> See (a bit outdated) comment in ScriptLexer.cpp.
> 
> // Our grammar of the linker script is LL(2), meaning that it needs at
> // most two-token lookahead to parse. The only place we need two-token
> // lookahead is labels in version scripts, where we need to parse "local :"
> // as if "local:".
> 
> Given above I'd go with explicit version:
> 
> ```
>     while (!errorCount() && peek() != ")" && peek() != "SORT" &&
>            peek() != "EXCLUDE_FILE")
> ```
> 
> Note:
> 1) That "(" is a valid file name. Your version goes into infinite loop with the following input (added the `FOO (` to script from test case):
> `# RUN: echo "SECTIONS { .abc : { *(SORT(.foo.*) .a* .a* SORT(.bar.*) FOO ( .b*) } }" > %t1.script`
> 2) Perhaps, the comment for `readInputSectionsList` should be updated.
> 
> 
Thanks for spotting the infinite loop problem! I added peekSortKind and changed back to LL(1).


================
Comment at: lld/test/ELF/linkerscript/sort2.s:8
 
-# CHECK:  Contents of section .abc:
-# CHECK:   01000000 00000000 02000000 00000000
-# CHECK:   03000000 00000000 04000000 00000000
-# CHECK:   06000000 00000000 05000000 00000000
+## FIXME Some input sections are duplicated in .abc and their second occurrences are zeros.
+# CHECK:      Hex dump of section '.abc'
----------------
grimar wrote:
> This is what is fixed by D91127, right?
Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91180