[PATCH] D50463: [llvm-objcopy] Add --prefix-sections option

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D50463#1193520, @jhenderson wrote:

> I've got some comments that I'll send through a bit later today, but as things stand, this doesn't match GNU objcopy's behaviour for relocation sections at the very least - the "prefix" is placed between the end of ".rela" and the start of the section name so that they have the name ".rela<prefixed relocated section name>"


It turns out that this is a wider bug in llvm-objcopy that is related to, but not specific to this issue: if you do --rename-section in GNU objcopy, you also implicitly rename relocation sections. Meanwhile, if you do --rename-section on relocation sections or strtab or symtab sections, nothing happens (if less concerned about this, because you requested something specific for those sections, but still, it is a behaviour difference).



================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:274
 
+static inline bool StartsWith(std::string S1, StringRef S2) {
+  return std::equal(S2.begin(), S2.end(), S1.begin());
----------------
This should take a pair of StringRefs, not a std::string and a StringRef. And then you can use StringRef::startswith.

Also `inline` has no purpose here. Compilers generally ignore it when it comes to function inlining. It does have a completely separate purpose in header files though.

It should also be named with lower-case (@jakehehrlich  and I discussed this offline, and I think that somewhere we started following a non-LLVM style for functions, contrary to the style guide, and we'd like to get back to the style it should be).


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:568
+    for (auto &Sec : Obj.sections()) {
+      if (Sec.Type != SHT_STRTAB && Sec.Type != SHT_SYMTAB)
+        Sec.Name = Config.SectionsPrefix.str() + Sec.Name;
----------------
I'm wondering if GNU objcopy's behaviour is section-name specific rather than type specific. Have you checked?


Repository:
  rL LLVM

https://reviews.llvm.org/D50463