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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:253
 
+  // We add a technical prefix for each unnamed section/fill. It does not affect
+  // the output, but allows us to map them by name in the code and
----------------
jhenderson wrote:
> Minor nit: comment flowing here seems a little weird (I'd expect "report" to be on the line before at least).
You refer to the name here as a prefix, but it's really a suffix. I think calling it a prefix is slightly confusing given it is supposed to be equivalent to the unique suffix used for identically-named sections.

Perhaps also worth mentioning that this should be identical style to the seciton name uniquing. In fact, if there's a way to enforce that identical property by code sharing, that would be even better.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:253-255
+  // We add a technical prefix for each unnamed section/fill. It does not affect
+  // the output, but allows us to map them by name in the code and
+  // report better error messages.
----------------
Minor nit: comment flowing here seems a little weird (I'd expect "report" to be on the line before at least).


================
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'
----------------
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).


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

https://reviews.llvm.org/D79984