[PATCH] D48146: ELF: Implement --icf=safe using address-significance tables.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall looking pretty good.



================
Comment at: lld/ELF/Driver.cpp:1202
 static void findKeepUniqueSections(opt::InputArgList &Args) {
+  auto KeepUnique = [](Symbol *S) -> bool {
+    if (auto *D = dyn_cast_or_null<Defined>(S)) {
----------------
nit: can't this return type inferred?

This function doesn't depend on anything in the outer function. I'd define this as a file-scope function.


================
Comment at: lld/ELF/Driver.cpp:1223
+
+    for (InputFile *F : ObjectFiles) {
+      auto *Obj = cast<ObjFile<ELFT>>(F);
----------------
Can you add a comment?


================
Comment at: lld/ELF/Driver.cpp:1235
+          if (Err)
+            fatal(F->getName() + ": could not decode addrsig section: " + Err);
+          KeepUnique(Syms[SymIndex]);
----------------
toString(F) is better because it automatically include an archive file name if F is created from an archive file.


================
Comment at: lld/ELF/ICF.cpp:176
   // Don't merge read only data sections unless
   // --ignore-data-address-equality was passed.
+  if (!(S->Flags & SHF_EXECINSTR) &&
----------------
Update this comment?


================
Comment at: lld/ELF/InputFiles.cpp:434
+          this->AddrsigSec = &Sec;
+        else if (Config->ICF == ICFLevel::Safe)
+          warn(toString(this) + ": --icf=safe is incompatible with object "
----------------
Maybe this needs some explanation as to why objcopy could break your object files.


https://reviews.llvm.org/D48146