[PATCH] D45195: Add --check-library-dependency to maintain compatibility with other linkers

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/Driver.cpp:934
+    case OPT_start_group:
+      ++InputFile::GroupDepth;
+      break;
----------------
grimar wrote:
> I think `GroupDepth` should not be a member of `InputFile`.
> It could be a local variable it seems:
> 
> ```
>   int32_t GroupDepth = 0;
>   for (auto *Arg : Args) {
>     switch (Arg->getOption().getUnaliasedOption().getID()) {
> ...
>     case OPT_start_group:
>       ++GroupDepth;
>       break;
>     case OPT_end_group:
>       if (GroupDepth > 0)
>         --GroupDepth;
>       else
>         error("stray --end-group");
>       break;
> ...
>     }
> 
>     // All files within the same --start-group and --end-group get the
>     // same group ID. Otherwise, a new file will get a new group ID.
>     if (!GroupDepth)
>       ++InputFile::CurrentGroupId;
>   }
> ```
We can't do that because we have to handle linker scripts as well.


================
Comment at: lld/ELF/Options.td:354
+defm check_library_dependency: B<"check-library-dependency",
+    "Do not allow backward symbol references",
+    "Allow backward symbol references">;
----------------
espindola wrote:
> To fetch archive members. Otherwise it sounds like bar.o cannot refer to foo.o  in "foo.o bar.o".
> 
Done


================
Comment at: lld/ELF/SymbolTable.cpp:292
+//
+// --check-library-dependency is an option to detect reverse or cyclic
+// dependencies between static archives, and it can be used to keep your
----------------
espindola wrote:
> This seems to be over selling it. It will not detect cyclic reference if the members that cause it are not used.
> 
> I think it is clear to just say that the option prevents an undefined reference from fetching a member written earlier in the command line.
> 
Done


================
Comment at: lld/ELF/SymbolTable.cpp:342
+  if (Config->CheckLibraryDependency && Old && New->GroupId < Old->GroupId)
+    warn("bad backward reference detected: " + Name + " in " + toString(Old) +
+         " depends on " + toString(New));
----------------
espindola wrote:
> I agree that "bad" is out of place. The user asked to find backwards references and we found one.
> 
> From the error it is also not clear that Name is a symbol name. How about something like
> 
> Undefined reference Name in Old refers to New.
> 
Changed the wording.


================
Comment at: lld/ELF/SymbolTable.cpp:378
+
+    checkDependency(Name, File, S->File);
+    fetchLazy<ELFT>(S);
----------------
espindola wrote:
> I think this is the only call to checkDependency that you need. We never find a backward reference when adding a lazy symbol.
Done


================
Comment at: lld/ELF/SymbolTable.cpp:295
+// your program compatible with GNU linkers after you switch to lld.
+// I'll explain the feature and why you may find it useful below.
+//
----------------
grimar wrote:
> I think our comments are usually impersonal.
I think "I" in this comment (in particular in this sentence) is fine.


https://reviews.llvm.org/D45195