[PATCH] D66799: [yaml2obj] - Allow placing local symbols after globals.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:998
 static bool buildSymbolsMap(ArrayRef<ELFYAML::Symbol> V, NameToIdxMap &Map) {
-  bool GlobalSymbolSeen = false;
-  std::size_t I = 0;
-  for (const ELFYAML::Symbol &Sym : V) {
-    ++I;
-
-    StringRef Name = Sym.Name;
-    if (Sym.Binding.value == ELF::STB_LOCAL && GlobalSymbolSeen) {
-      WithColor::error() << "Local symbol '" + Name +
-                                "' after global in Symbols list.\n";
-      return false;
-    }
-    if (Sym.Binding.value != ELF::STB_LOCAL)
-      GlobalSymbolSeen = true;
-
-    if (!Name.empty() && !Map.addName(Name, I)) {
-      WithColor::error() << "Repeated symbol name: '" << Name << "'.\n";
-      return false;
-    }
+  for (size_t I = 0; I < V.size(); ++I) {
+    const ELFYAML::Symbol &Sym = V[I];
----------------
jhenderson wrote:
> Maybe: `for (size_t I = 0, S = V.size(); I < S; ++I)`
Ok.


================
Comment at: test/tools/yaml2obj/elf-symbols-binding-order.yaml:5
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readelf --symbols --dyn-symbols %t | FileCheck %s
+
----------------
jhenderson wrote:
> MaskRay wrote:
> > --symbols dump both .symtab and .dynsym.
> > 
> > I think you can change the CHECK lines for .dynsym so that you can:
> > 
> > 1) test --dyn-symbols on .dynsym
> > 2) test --symbols on both .symtab and .dynsym
> As this is just a yaml2obj test, I think we can just remove "--dyn-symbols". There would be no need to change the CHECK lines then.
I removed `--dyn-symbols`.


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

https://reviews.llvm.org/D66799