[PATCH] D50381: [llvm-objcopy] Add --prefix-symbols option

Paul Semel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 7 09:27:14 PDT 2018


paulsemel marked an inline comment as done.
paulsemel added inline comments.


================
Comment at: test/tools/llvm-objcopy/prefix-symbols.test:22-24
+    - Name:     foo
+      Type:     STT_SECTION
+      Section:  .text
----------------
jhenderson wrote:
> This is a little weird. Section symbols don't usually have a name, since they effectively have the section's name. Does yaml2obj allow you to create a section symbol for a specific section (and thus end up with st_name == 0)?
Does it really matter ? We want to show that the name is unchanged, and indeed `yaml2obj` is puting a name for this section symbols, so..


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:215
 
+static std::vector<std::unique_ptr<std::string>> SymbolsPrefixedNames;
+
----------------
jhenderson wrote:
> paulsemel wrote:
> > This might not be the correct way to retain the new symbols names (and surely not the correct place to have it anyway), but I couldn't find a smarter way to do it..
> The lazy answer would be to just give ownership of symbol names to the symbol class. I don't know if that's the right thing to do, but it feels a bit better than this to me.
Well, not really, as a symbol shouldn't have anything else than a StringRef (as it is to be immutable). The fact is that we need to have a "new" string in memory, to be referenced by this symbol.. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:372
+        SymbolsPrefixedNames.emplace_back(
+            new std::string(Config.SymbolsPrefix.str() + Sym.Name.str()));
+        Sym.Name = *SymbolsPrefixedNames.back();
----------------
jhenderson wrote:
> There's not much point in newing a std::string into existence. Just create a vector of strings.
No, with vector of strings, the address can change (using vectors, we can't ensure the address of elements will remain the same)


Repository:
  rL LLVM

https://reviews.llvm.org/D50381





More information about the llvm-commits mailing list