[PATCH] D23552: [ELF] - Give automatically generated __start_* and __stop_* symbols default visibility.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/SymbolTable.cpp:425
@@ -424,3 +424,3 @@
   std::tie(S, WasInserted) =
-      insert(N, STT_NOTYPE, STV_HIDDEN, /*CanOmitFromDynSym*/ false,
+      insert(N, STT_NOTYPE, StOther & 3, /*CanOmitFromDynSym*/ false,
              /*IsUsedInRegularObj*/ true, nullptr);
----------------
grimar wrote:
> ruiu wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > I'm not the type of person who wants to eliminate all constants, but 3? It's too magical.
> > > Actually it is used in many places in lld. I count 4 more places in this file (SymbolTable.cpp) and also once in Symbols.h:
> > > 
> > > ```
> > > uint8_t getVisibility() const { return StOther & 0x3; }
> > > ```
> > > 
> > > I suggest to leave it for now and fix once for all places if you want it.
> > The use of `0x3` in `getVisibility` is fine because it is obvious that it is extracting visibility bits. It actually hides the use of `0x3` and wraps it with a better name (getVisibility). But in this context there's no information as to what you are doing.
> What about adding comment then ?
> 
> ```
>   bool WasInserted;
>   std::tie(S, WasInserted) =
>       insert(N, STT_NOTYPE, StOther & 0x3 /*Visibility*/, /*CanOmitFromDynSym*/ false,
> ```
> 
> Or do you prefer static method for that ?
> 
> 
> ```
> static unsigned getVisibility(unsigned O) { 
>   return O & 0x3;
> }
> 
> ...
> 
>   std::tie(S, WasInserted) =
>       insert(N, STT_NOTYPE, getVisibility(StOther), /*CanOmitFromDynSym*/ false,
> ```
A quick google search showed me that there's macros to get the value, ELF32_ST_VISIBILITY and ELF64_ST_VISIBILITY. Unfortunately they are as confusing as the mask because of ELF32/ELF64 prefixes (although what they do is the same.) So I'd go with a comment.


https://reviews.llvm.org/D23552