[PATCH] D43126: [LLD][ELF] Do not error for missing version when symbol has local version.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

>> Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:
>>  Index: ELF/Symbols.cpp
>>  ===================================================================
>> 
>>   - ELF/Symbols.cpp +++ ELF/Symbols.cpp @@ -206,8 +206,10 @@ // It is an error if the specified version is not defined. // Usually version script is not provided when linking executable, // but we may still want to override a versioned symbol from DSO,
>> - // so we do not report error in this case.
>> - if (Config->Shared) +  // so we do not report error in this case. We also do not error +  // if the symbol has a local version as it won't be in the dynamic +  // symbol table. +  if (Config->Shared && VersionId != VER_NDX_LOCAL)
> 
> Is the Config->Shared part still required?

At the moment it is; I get the error messages I remove it. If we could somehow arrange the default VersionId for symbols in executables to have VersionId == VER_NDX_LOCAL then the Config->Shared could be dropped. I can investigate that if you'd prefer it?

> Should an error be reported when linking an executable with
>  --export-dynamic?

A good question. In theory it should as the symbol is exported, in practice I fear that the likelihood of anyone using export-dynamic with a @version definition is lower than the likelihood of using export-dynamic in a large project with a lurking @version definition that they did not know about (like the Bionic case). It seems like there isn't much consistency with how this is handled, and gold doesn't seem to handle the case sensibly. Table renders best in phab:

| Linker | With version script              | Without version script                        |
| bfd    | symbol given version from script | symbol not given version                      |
| gold   | symbol given global version      | version defined, but symbol not given version |
| lld    | symbol given version from script | symbol not given version                      |
|

In summary, I don't have a strong opinion either way but I'm leaning towards not giving an error due to the risk of breaking existing programs.


https://reviews.llvm.org/D43126