[PATCH] D35724: [ELF] - Fix missing relocation when linking executable with --unresolved-symbols=ignore-all

[George Rimar via llvm-commits]

>    // Only symbols that appear in dynsym can be preempted.
>>    if (!symbol()->includeInDynsym())
>>      return false;
>>
>>    // Only default visibility symbols can be preempted.
>>    if (symbol()->Visibility != STV_DEFAULT)
>>      return false;
>>
>> +  // Undefined symbols in non-DSOs are usually just an error, so it
>> +  // doesn't matter whether we return true or false here. However, if
>> +  // -unresolved-symbols=ignore-all is specified, undefined symbols in
>> +  // executables are automatically exported so that the runtime linker
>> +  // can try to resolve them. In that case, they is preemptible. So, we
>> +  // return true for an undefined symbol in case the option is specified.
>> +  if (!Config->Shared)
>> +    return isUndefined() && !symbol()->isWeak();
>
>The change in this patch is to add "!symbol()->isWeak();"
>above. Shouldn't this case have been handled by includeInDynsym two ifs above?

Not for --export-dynamic case. includeInDynsym() will return true earlier.

>>  bool Symbol::includeInDynsym() const {
>>    if (computeBinding() == STB_LOCAL)
>>      return false;
>> -  return ExportDynamic || body()->isShared() ||
>> -         (body()->isUndefined() && Config->Shared);
>> +  if (ExportDynamic || body()->isShared())
>> +    return true;
>> +  if (!body()->isUndefined())
>> +    return false;
>> +  return Config->Shared || !body()->symbol()->isWeak();
>
>Can you change the last line to just "return Config->Shared"?
>
>Cheers,
>Rafael

No, because with just "return Config->Shared" includeInDynsym()
will return false for any undefined symbol in non-DSO. And after that
isPreemtible() will exit early.

Did you mean change to just "return true" ? That will not work because breaks
weak-undef.s test which checks that we do not include weak undefined symbols
into non-DSO.

George.