[PATCH] D19400: ELF: Move Visibility, IsUsedInRegularObj and MustBeInDynSym flags to Symbol.

[Rui Ueyama via llvm-commits]

ruiu added a comment.

This is a very interesting idea. I have never thought of adding new fields to Symbol, but it seems to make sense because the argument that some properties belongs to names rather than to symbol instances is convincing. One concern would be that now you have to memorize which class have a certain property, Symbol or SymbolBody, but that's probably not a big issue.

There are a few minor comments.


================
Comment at: ELF/LTO.cpp:79
@@ -78,3 +78,3 @@
                               SymbolBody &B, GlobalValue *GV) {
-  if (B.isUsedInRegularObj())
+  if (B.getSymbol()->IsUsedInRegularObj)
     return false;
----------------
Maybe we should make Backref member public then use that member directly and remove getSymbol accessor?

================
Comment at: ELF/MarkLive.cpp:116-121
@@ -115,9 +115,8 @@
   // file can interrupt other ELF file's symbols at runtime.
   if (Config->Shared || Config->ExportDynamic) {
     for (const Symbol *S : Symtab->getSymbols()) {
-      SymbolBody *B = S->Body;
-      if (B->includeInDynsym())
-        MarkSymbol(B);
+      if (S->includeInDynsym())
+        MarkSymbol(S->Body);
     }
   }
 
----------------
You can remove all curly braces.

================
Comment at: ELF/SymbolTable.cpp:290
@@ -288,1 +289,3 @@
   New->setBackref(Sym);
+  // Merge in the new symbol's visibility. Visibility of DSO symbols does not
+  // affect visibility in the output.
----------------
Please add a newline before this line.

================
Comment at: ELF/SymbolTable.cpp:294-302
@@ +293,11 @@
+    Sym->Visibility = getMinVisibility(Sym->Visibility, New->getVisibility());
+  if (Config->Shared || Config->ExportDynamic) {
+    // Export most symbols except for those that do not need to be exported.
+    if (!New->CanOmitFromDynSym)
+      Sym->ExportDynamic = true;
+  } else {
+    // Make sure we preempt DSO symbols with default visibility.
+    if (New->isShared() && New->getVisibility() == STV_DEFAULT)
+      Sym->ExportDynamic = true;
+  }
+  auto K = New->kind();
----------------
Can you move this code to a new static function (say, shouldExport) so that you can write

  Sym->ExportDynamic ||= shouldExport(New);

================
Comment at: ELF/SymbolTable.cpp:303
@@ +302,3 @@
+  }
+  auto K = New->kind();
+  if (K == SymbolBody::DefinedRegularKind ||
----------------
s/K/SymbolBody::Kind/

================
Comment at: ELF/Symbols.cpp:327
@@ -365,2 +326,3 @@
+  uint8_t V = Visibility;
   if (V != STV_DEFAULT && V != STV_PROTECTED)
     return false;
----------------
V is used only in this line, so probably

   if (Visibility != STV_DEFAULT && Visibility != STV_PROTECTED)

is better.


http://reviews.llvm.org/D19400