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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Overall looks good to me. I've got a few suggestions on the refactoring. One other thing that might be worth doing to make this easier to review is to split it into two patches:

- A refactoring change that doesn't change the wildcard behaviour.
- The change to the wildcard behaviour.

Not got a strong opinion on that now I've gone through it.



================
Comment at: ELF/Config.h:75
+  uint16_t id;
+  std::vector<SymbolVersion> names;
 };
----------------
names is very close to name. Perhaps patterns as this is what I think a SymbolVersion represents. For example foo*. Come to think of it, SymbolVersion could be confusing as it is more like a VersionPattern. 


================
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});
----------------
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.


================
Comment at: ELF/ScriptParser.cpp:1347
   std::tie(locals, globals) = readSymbols();
-
-  for (SymbolVersion v : locals) {
-    if (v.name == "*")
-      config->defaultSymbolVersion = VER_NDX_LOCAL;
-    else
-      config->versionScriptLocals.push_back(v);
-  }
-
-  for (SymbolVersion v : globals)
-    config->versionScriptGlobals.push_back(v);
+  llvm::copy(locals, std::back_inserter(config->versionDefinitions[0].names));
+  llvm::copy(globals, std::back_inserter(config->versionDefinitions[1].names));
----------------
This file is where I was thinking that versionDefinitions[VER_NDX_LOCAL].names would be clearer for 0, and versionDefinitions[VER_NDX_GLOBAL] for 1.


================
Comment at: ELF/ScriptParser.cpp:1367
 
   // User-defined version number starts from 2 because 0 and 1 are
   // reserved for VER_NDX_LOCAL and VER_NDX_GLOBAL, respectively.
----------------
Is this comment needed now that the +2 has been removed.


================
Comment at: ELF/SymbolTable.cpp:207
+    if (sym->verdefIndex == UINT32_C(-1)) {
+      sym->verdefIndex = 0;
       sym->versionId = versionId;
----------------
Would prefer verdefIndex = VER_NDX_LOCAL;


================
Comment at: ELF/SyntheticSections.cpp:2775
+  for (const VersionDefinition &v :
+       makeArrayRef(config->versionDefinitions).slice(2))
     verDefNameOffs.push_back(getPartition().dynStrTab->addString(v.name));
----------------
Any reason why in Sybols.cpp auto & was used instead of the const VersionDefinition & used here?

Maybe worth a named helper function that returns makeArrayRef(config->versionDefinitions).slice(2)); ? Looking at this line in isolation it is not obvious why?


================
Comment at: ELF/Writer.cpp:399
 
-      if (!config->versionDefinitions.empty()) {
+      if (config->versionDefinitions.size() > 2) {
         part.verDef = make<VersionDefinitionSection>();
----------------
A similar comment to the slice(2). It isn't obvious from the line why the > 2 is here. Not a big problem but possibly worth a comment. 


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D65716