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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

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



================
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;
----------------
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.


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

https://reviews.llvm.org/D60122