[PATCH] D89004: [LLD] [COFF] Implement a GNU/ELF like -wrap option

Wed Oct 14 09:19:02 PDT 2020

rnk accepted this revision.
rnk added inline comments.
This revision is now accepted and ready to land.

================
Comment at: lld/COFF/Symbols.h:141
+  // doesn't know the final contents of the symbol.
+  uint8_t canInline : 1;
+
----------------
MaskRay wrote:
> mstorsjo wrote:
> > rnk wrote:
> > > There are more kinds of IPO than just inlining, so the name isn't quite right. I also would prefer to reverse the sense so that a default value of false is the more prevalent value, and only special wrapped symbols set this to true. I suggest using the name `isHiddenFromLTO` for that purpose.
> > Sounds like a good suggestion, but this is (including the comment) an exact copy of the corresponding field from the ELF side - and consistency also is nice.
> > 
> > Or should I go with this and then rename accordingly on the ELF side afterwards - WDYT @MaskRay?
> I agree that canInline is a misnomer. `isHiddenFromLTO` may be a bit confusing as well: there is an LTO property called VisibleToRegularObj. "hidden" and "Visible" are confusing when used together. FWIW in lib/LTO, this property is called `LinkerRedefined`.
> 
> However, `LinkerRedefined` is not great for the ELF case because a linker script symbol assignment (e.g. `a = 1013;`) does not set `LinkerRedefined`. The ELF port just overrides the LTO definition in a later pass.
I think consistency with ELF is better than trying to improve on the naming. We can do a follow-up to rename all the properties to something sensible and consistent.

For example, I like `IsUsedByRegularObj` better than `VisibleFromRegularObj`, and I think we can improve on `LinkerRedefined`, and make that match whatever LLD uses in both ELF and COFF.

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

https://reviews.llvm.org/D89004