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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 4 inline comments as done.
MaskRay 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)
----------------
peter.smith wrote:
> 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.
Just refactored the code a bit to dispatch on symbol kinds:

if (undefined) {
} else if (SharedSymbol) {
} else if (lazy) {
}

I hope the logic will become clearer.


================
Comment at: ELF/Symbols.h:370
+  // change to STB_WEAK if the first undefined reference is weak.
+  bool BindingFinalized = false;
 };
----------------
peter.smith wrote:
> 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;
> ```
> 
Thanks for the suggestions! I'll go with `Referenced` (preferring positive forms to avoid double negative).


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D63974