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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/Driver.cpp:934
+    case OPT_start_group:
+      ++InputFile::GroupDepth;
+      break;
----------------
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;
  }
```


================
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.
+//
----------------
I think our comments are usually impersonal.


================
Comment at: lld/ELF/SymbolTable.cpp:339
+//
+// A and B form group 0, C, D and their member object files form group
+// 1, and E forms group 2. I think that you can see how this group
----------------
I would end the sentence after "group 0." 
Otherwise "0, C, D" looks like a single list.


================
Comment at: lld/ELF/SymbolTable.cpp:341
+// 1, and E forms group 2. I think that you can see how this group
+// assignment rule simulates the traditional linker's semantics.
+static void checkDependency(StringRef Name, InputFile *Old, InputFile *New) {
----------------
"I think that you can see how this group assignment rule simulates the traditional linker's semantics."

Is also personal and does not make sense in the comment. It brings no new information about the feature.
I would remove this sentence completely.


================
Comment at: lld/ELF/SymbolTable.cpp:344
+  if (Config->CheckLibraryDependency && Old && New->GroupId < Old->GroupId)
+    error("bad backward reference detected: " + Name + " in " +
+          toString(Old) + " depends on " + toString(New));
----------------
Why is it "bad"? In the policy of LLD it is valid and hence neutral. It just that old linkers
does not support such references, though that do not make them "bad". I would drop this word.


================
Comment at: lld/ELF/SymbolTable.cpp:344
+  if (Config->CheckLibraryDependency && Old && New->GroupId < Old->GroupId)
+    error("bad backward reference detected: " + Name + " in " +
+          toString(Old) + " depends on " + toString(New));
----------------
grimar wrote:
> Why is it "bad"? In the policy of LLD it is valid and hence neutral. It just that old linkers
> does not support such references, though that do not make them "bad". I would drop this word.
Also, should it be a warning? It is a bit more flexible probably.


================
Comment at: lld/ELF/SymbolTable.cpp:613
+  if (InputFile *File = F.fetch(Sym)) {
+    checkDependency(Name, S->File, File);
     addFile<ELFT>(File);
----------------
`checkDependency` duplicates in few places, I would suggest
to land D45083 first and I think you should be able to use  `SymbolTable::fetchIfLazy` here and
move `checkDependency` there to avoid duplications.


https://reviews.llvm.org/D45195