[PATCH] D39242: [ELF] - Stop naming relocation sections with first input section name.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Writer.cpp:101
 
+  // This is normally used for --emit-relocs. With that we want
+  // to name relocation section similar to it's target section.
----------------
ruiu wrote:
> Is there an "abnormal" situation in which this code is used even if --emit-relocs is not given?
Yes. Since this code called for synthetic sections, it executes for ".rela.dyn" for example.
It looks to be safe, as it returns the same result as original code for that case, so does
not seem we need additional checks here.


================
Comment at: ELF/Writer.cpp:104-110
+  for (StringRef V : {".rel.", ".rela."}) {
+    if (!Name.startswith(V))
+      continue;
+    StringRef Prefix = V.drop_back();
+    StringRef Target = getOutputSectionName(Name.drop_front(Prefix.size()));
+    return Saver.save(Prefix + Target);
+  }
----------------
ruiu wrote:
> Is this the same as this?
> 
>   if (Name.startswith(".rel."))
>     return Saver.save(".rel." + getOutputSectionName(Name.substr(4)));
>   if (Name.startswith(".rela."))
>     return Saver.save(".rela." + getOutputSectionName(Name.substr(5)));
That was something close to what I did in the first diff. I do not think we want to do that.
`getOutputSectionName` sometimes returns value from string saver, or for "COMMON" case it returns
".bss". So it is not consistent and not always returns the part of the same string.

I am not sure how much is possible to create .rela.COMMON or .rela.zdebug_ section, but I think we should not
write code that assumes it is impossible, as it is too complicated assumption which needs additional testing probably. 
I believe my version is much more straightforward and the fact it rarely allocates the string is not a great issue.


https://reviews.llvm.org/D39242