[PATCH] D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:587
+  return ".data.rel.ro";
+}
+
----------------
bd1976llvm wrote:
> nickdesaulniers wrote:
> > is this section prefixing specific to ELF, or common between all object file formats? Should the function name contain `ELF` if specific to ELF?
> > 
> > Is there a more specific guard we can use for `".data.rel.ro"`, returning an empty string otherwise?
> > is this section prefixing specific to ELF, or common between all object file formats? Should the function name contain ELF if specific to ELF?
> 
> This function is also used by web assembly which is why it doesn't have ELF in the name.
> 
> > Is there a more specific guard we can use for ".data.rel.ro", returning an empty string otherwise?
> 
> An llvm_unreachable would make this code clearer. I'll add one.
> 
> 
This looks funny; an `assert`, followed by an indented `return`, followed by an `llvm_unreachable`?  Was the `assert` supposed to be an `if` statement?


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

https://reviews.llvm.org/D72194