[PATCH] D79984: [yaml2obj] - Add a technical prefix for each unnamed chunk.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-link.yaml:32
+# ERR:      error: unknown section referenced: '.unknown1' by YAML section '.foo'
+# ERR-NEXT: error: unknown section referenced: '.unknown2' by YAML section ' [unnamed, index #2]'
+# ERR-NEXT: error: unknown section referenced: '.unknown3' by YAML section '.bar'
----------------
jhenderson wrote:
> grimar wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > MaskRay wrote:
> > > > > Can we omit `unnamed` and use `'' (index 2)`?
> > > > We can omit/add whatever we want into `[ ... ]` part.
> > > > 
> > > > This patch works because it automatically adds unique suffixes. What we use for
> > > > describing sections/symbols with the same name already:
> > > > 
> > > > ```
> > > >   - Name: '.foo [1]'
> > > >     Type: SHT_PROGBITS
> > > >   - Name: '.foo [2]'
> > > >     Type: SHT_PROGBITS
> > > > ```
> > > > 
> > > > I.e. we can add any `[ any text here ]` as suffix to a section name and the existent logic will
> > > > handle it properly. But we can't switch `[]` to `()` without doing additional changes.
> > > > 
> > > > It could be `' [index #2]'`, but not `'' [index 2]` nor `'' (index 2)`.
> > > > 
> > > > It can be possible to change (add an additional name parsing of a section name) in 
> > > > `toSectionIndex` to improve the message reported, but I am not sure it worth that additional complexity:
> > > > 
> > > > ```
> > > > template <class ELFT>
> > > > unsigned ELFState<ELFT>::toSectionIndex(StringRef S, StringRef LocSec,
> > > >                                         StringRef LocSym) {
> > > > ....
> > > > //// Add a code to parse LocSec/LocSym here.
> > > > 
> > > >   if (!LocSym.empty())
> > > >     reportError("unknown section referenced: '" + S + "' by YAML symbol '" +
> > > >                 LocSym + "'");
> > > >   else
> > > >     reportError("unknown section referenced: '" + S + "' by YAML section '" +
> > > >                 LocSec + "'");
> > > >   return 0;
> > > > }
> > > > ```
> > > OK, this name should be fine. But why doesn't `'' [index 2]"` work? (I am actually ok with either one but `#2` not working seems strange to me.)
> > See.
> > 
> > 1) We have the following code:
> > 
> > ```
> > StringRef llvm::ELFYAML::dropUniqueSuffix(StringRef S) {
> >   size_t SuffixPos = S.rfind(" [");
> >   if (SuffixPos == StringRef::npos)
> >     return S;
> >   return S.substr(0, SuffixPos);
> > }
> > ```
> > 
> > I.e. we can safely add ` [` suffix and everything starting from it will be ignored.
> > The empty section name with a suffix looks like " [...", where `...` is any data or nothing.
> > e.g " [ index 2 ]"/" [ foo"/" [" are valid empty section names.
> > 
> > 2)  The code that reports an error is:
> > ```
> > reportError("unknown section referenced: '" + S + "' by YAML section '" +
> >                 LocSec + "'");
> > ```
> > 
> > I.e. if the section name is `X`, it prints:
> > ```
> > unknown section referenced: '.foo' by YAML section 'X' 
> > ```
> > 
> > How can it print `'' [index 2]`? For section name " [ index 2 ]" it would print:
> > 
> > ```
> > unknown section referenced: '.foo' by YAML section ' [index 2]`
> > ```
> > 
> > 
> I can probably live with the bit in square brackets being inside the quotes, but is it possible to get rid of the extra space at least? That might require a little extra logic in `dropUniqueSuffix` to work, but I think it might be a nice property, and might even be more generally useful (allowing slight variations in whitespace in unique names without affecting behaviour).
> but is it possible to get rid of the extra space at least?

It is problematic because we have at least 2 tests that doesn't like it:
https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/llvm-objcopy/ELF/wildcard-syntax.test#L111
https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/yaml2obj/macro.yaml#L57

The best solution I see is to start using "<>" brackets instead of "[]" for unique suffixes.

I made a code change in this patch to show how it might work. So what about if I change `[]`->`<>` first?


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

https://reviews.llvm.org/D79984