[PATCH] D66091: [ELF] Simplify handling of exportDynamic and isPreemptible

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

The code changes look good to me. I've made some suggestions on the name of the flag and some of the comments.



================
Comment at: ELF/InputFiles.cpp:1485
 
   Defined New(&f, name, binding, visibility, type, 0, 0, nullptr);
+  New.canOmitFromDynSym = canOmitFromDynSym;
----------------
Post variable naming change, could we use newDefined or newSym to avoid the New (presumably to avoid clash with new).


================
Comment at: ELF/Symbols.h:119
 
-  // If this flag is true and the symbol has protected or default visibility, it
-  // will appear in .dynsym. This flag is set by interposable DSO symbols in
-  // executables, by most symbols in DSOs and executables built with
-  // --export-dynamic, and by dynamic lists.
+  // Used by Defined with protected or default visibility to track if it should
+  // appear in .dynsym. This flag is set by interposable DSO symbols (symbols
----------------
I've struggled with the name and comment here. I think it is because this is really the union of two separate properties that have a similar effect, with a similar name to a command line option that has a related but different effect. The two separate properties are:
- In an executable, there is a Shared symbol with default visibility that we may preempt or define.
- The symbol is present in a dynamic list.
Separate from this we have --export-dynamic which affects whether all symbols of the appropriate visibility defined in an executable should be put into the dynamic symbol table. The presence of --export-dynamic on the command line does not affect the exportDynamic flag in Symbol even though the name suggests that it should.

The best I can come up with for alternative is something like requiresDynamic or forceDynamic. 
```
requiresDynamic is used by a Defined symbol with protected or default visibility, to record whether a symbol is required to be exported into the .dynsym. This is set when any of the following conditions hold:
- In an executable, when there is an existing Shared symbol with default visibility from an interposable DSO.
- When the symbol is present in the dynamic list file.
```


================
Comment at: ELF/Writer.cpp:1659
 
+  // In a DSO, the dynamic list specifies preemptible symbols.
+  if (config->hasDynamicList)
----------------
A small nit, the comment implies that the dynamic list is mandatory.
// In a DSO, if the symbol is in a dynamic list then it specifies whether the symbol is preemptible.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D66091