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

[George Rimar via llvm-commits]

grimar 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);
----------------
ruiu wrote:
> 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.
Yes, that is why I did not use them + also do not sure if we were using macroses in lld before.


https://reviews.llvm.org/D23552