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

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: ELF/Symbols.h:370
+  // change to STB_WEAK if the first undefined reference is weak.
+  bool BindingFinalized = false;
 };
----------------
MaskRay wrote:
> 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).
FWIW

> This is false if no undefined reference has been seen yet

Peter's version which avoided using negation looks a bit simpler to me:
"This is true if there has been at least one undefined reference to the symbol"


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D63974