[PATCH] D60122: [yaml2obj][obj2yaml] - Change how symbol's binding is descibed when parsing/dumping.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added a comment.

In D60122#1452354 <https://reviews.llvm.org/D60122#1452354>, @rupprecht wrote:

> LGTM, but since this is a pretty big change, it'd be good to get someone else's opinion


Yeah. Thanks, Jordan!



================
Comment at: tools/yaml2obj/yaml2elf.cpp:807-808
+    StringRef Name = Sym.Name;
+    if (Sym.Binding.value == ELF::STB_LOCAL && GlobalSymbolSeen) {
+      WithColor::error() << "Has a local symbol '" + Name + "' after global.\n";
+      return false;
----------------
rupprecht wrote:
> grimar wrote:
> > rupprecht wrote:
> > > Why does this need to be an error?
> > To always produce the correct ELF output for now:
> > "In each symbol table, all symbols with STB_LOCAL binding precede the weak and global symbols."
> > (http://refspecs.linuxbase.org/elf/gabi4+/ch4.symtab.html)
> > 
> > Before this change, we followed this rule. And the current code does the same.
> > 
> > I think we might want to remove this restriction soon (to allow producing broken outputs),
> > but I just did not want to do it in this patch. That would require adding more test case(s) I think
> > to document our behavior in different situations.
> > 
> > Also atm `findLocalsNum` I added to yaml2elf.cpp assumes that locals always go first for simplicity.
> > 
> Thanks, that makes sense. I wasn't aware of that restriction.
> 
> When you do want to break it, it would be great if the *default* mode was to be strict like this, and require additional syntax when you want to create a broken elf file.
> When you do want to break it, it would be great if the *default* mode was to be strict like this, and require additional syntax when you want to create a broken elf file.

In recent reviews of yaml2obj/obj2yaml we briefly discussed an idea about adding one more flag, `-no-validate` or alike,
which would allow disabling validation of the output. Perhaps this is one more case when such a flag might be useful.


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

https://reviews.llvm.org/D60122