[PATCH] D63974: [ELF] Only allow the binding of SharedSymbol to change for the first undef ref

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: ELF/Symbols.cpp:413
 
-  if (isShared() || isLazy() || (isUndefined() && Other.Binding != STB_WEAK))
+  if (auto *S = dyn_cast<SharedSymbol>(this)) {
+    if (Other.Binding != STB_WEAK || !S->BindingFinalized)
----------------
Coming at this part of the code with fresh eyes, I think a comment, or reference to a comment elsewhere, explaining why we are setting the binding of an existing shared symbol to weak would be helpful here. I managed to find one in Symbols.h that helped but I had to search for it:

```
  // Symbol binding. This is not overwritten by replace() to track
  // changes during resolution. In particular:
  //  - An undefined weak is still weak when it resolves to a shared library.
  //  - An undefined weak will not fetch archive members, but we have to
  //    remember it is weak.
```

One other thing that might be worth mentioning is why lazy symbols don't need the same treatment. As I understand it a non-weak reference would immediately fetch the member to define the symbol so there isn't a case where the undefined weak reference changes the binding.


================
Comment at: ELF/Symbols.h:368
+
+  // This is true if no undefined reference has been seen yet. The binding may
+  // change to STB_WEAK if the first undefined reference is weak.
----------------
I'd have thought it was false if no undefined reference has been seen yet?


================
Comment at: ELF/Symbols.h:370
+  // change to STB_WEAK if the first undefined reference is weak.
+  bool BindingFinalized = false;
 };
----------------
I think BindingFinalized implies that it never changes but it can if we see a weak refrence first and then see a non-weak reference. As I understand the implementation, this will be set to true on first reference of the shared symbol. Some possible alternatives:
```
// This is true if no undefined reference to the symbol has been seen. We use this 
bool Unreferenced = true;
```
Note: this would need the logic reversed in the rest of the code.
```
if (Other.Binding != STB_WEAK || S->Unreferenced)
      Binding = Other.Binding;
S->Unreferenced = false;
```
Alternative retaining the existing logic.
```
// This is true if there has been at least one undefined reference to the symbol 
bool Referenced = false;
```



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D63974