[PATCH] D65716: [ELF] Consistently priority non-* wildcards overs "*" in version scripts
Peter Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 5 03:13:42 PDT 2019
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
More information about the llvm-commits
mailing list