[PATCH] D65716: [ELF] Consistently priority non-* wildcards overs "*" in version scripts

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Generally I also think it is fine.
A few comments from me (some of them seems were already mentioned by Peter).



================
Comment at: ELF/Driver.cpp:1021
   if (auto *arg = args.getLastArg(OPT_retain_symbols_file)) {
-    config->defaultSymbolVersion = VER_NDX_LOCAL;
+    config->versionDefinitions[0].names.push_back(
+        {"*", /*isExternCpp=*/false, /*hasWildcard=*/true});
----------------
peter.smith wrote:
> Would it be clearer to say [VER_NDX_LOCAL] instead of [0] and [VER_NDX_GLOBAL] instead of 1? In this file it is quite easy to see what 0 and 1 are from the push_back above, but this doesn't necessarily hold for other files.
What about accessing here and elsewhere as `config->versionDefinitions[VER_NDX_LOCAL]` and `config->versionDefinitions[VER_NDX_GLOBAL]`? I.e. idea is to get rid of magic values + increase the readability.


================
Comment at: ELF/ScriptParser.cpp:1348
+  llvm::copy(locals, std::back_inserter(config->versionDefinitions[0].names));
+  llvm::copy(globals, std::back_inserter(config->versionDefinitions[1].names));
 
----------------
During my work on LLD I was often asked to avoid using STL algorithms in favor of straightforward loops.

Perhaps I would do this here too now, for consistency with the rest of LLD code.


================
Comment at: ELF/SyntheticSections.cpp:1166
+// Returns the number of entries in .gnu.version_d: the number of
+// non-VER_NDX_LOCAL-non-VER_NDX_GLOBAL definitions, plus 1. Note that we don't
+// support vd_cnt > 1 yet.
----------------
`, plus 1` looks a bit strange here IMO because code has `-1`.
I understand this is because of "-2 + 1 == -1", but would be better if comment described this explicitly probably.
(i.e. it is assumed that reader knows there are exactly 2 more (VER_NDX_LOCAL + VER_NDX_GLOBAL) definitions in this array,
but this is not obvious, probably). Though may be it just me, so feel free to ignore.



================
Comment at: ELF/Writer.cpp:399
 
-      if (!config->versionDefinitions.empty()) {
+      if (config->versionDefinitions.size() > 2) {
         part.verDef = make<VersionDefinitionSection>();
----------------
Needs a comment about what is 2.


================
Comment at: test/ELF/version-script-reassign-glob.s:16
+
+# BAR:   GLOBAL DEFAULT 7 foo@@bar
+
----------------
`CHECK` -> `FOO` probably for consistency.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D65716