[llvm-commits] [llvm] r115753 - /llvm/trunk/lib/Linker/LinkModules.cpp

[Bill Wendling]

On Oct 5, 2010, at 11:38 PM, Eric Christopher wrote:

> On Oct 5, 2010, at 11:16 PM, Bill Wendling wrote:
> 
>> +// RequiresMerge - Return true if the source global variable needs to be merged
>> +// with the destination global variable. I.e., there shouldn't be a new global
>> +// variable created in the destination Module, rather the initializers should be
>> +// merged in an intelligent fashion.
>> +static bool RequiresMerge(const GlobalVariable *SGV, const GlobalVariable *DGV){
>> +  const StringRef SrcSec(SGV->getSection());
>> +  const StringRef DstSec(DGV->getSection());
>> +
>> +  // The Objective-C __image_info section should be unique.
>> +  if (SrcSec == DstSec &&
>> +      (SrcSec.find("__objc_imageinfo") != StringRef::npos ||
>> +       SrcSec.find("__image_info") != StringRef::npos))
>> +    return true;
>> +
>> +  return false;
>> +}
>> +
> 
> Definitely think this should be "RequiresUnique" instead of RequiresMerge, perhaps
> UniqueSection(SGV->getSection(), DGV->getSection())?  Merging sounds too much
> like the ELF SEC_MERGE or general merging of data.  
> 
>> @@ -819,10 +848,10 @@
>>      // Grab destination global variable or alias.
>>      GlobalValue *DGV = cast<GlobalValue>(ValueMap[SGV]->stripPointerCasts());
>> 
>> -      // If dest if global variable, check that initializers match.
>> +      // If dest is a global variable, check that initializers match.
>>      if (GlobalVariable *DGVar = dyn_cast<GlobalVariable>(DGV)) {
>>        if (DGVar->hasInitializer()) {
>> -          if (SGV->hasExternalLinkage()) {
>> +          if (SGV->hasExternalLinkage() || RequiresMerge(SGV, DGVar)) {
>>            if (DGVar->getInitializer() != SInit)
>>              return Error(Err, "Global Variable Collision on '" +
>>                           SGV->getName() +
> 
> It'll make this verification here sound like it makes more sense.
> 
> Thoughts?
> 
As for the nomenclature, I don't really have a strong opinion. You're correct that (at least for this particular bug) we want the initializers to be unique, instead of merging them. I used "merge" to leave open the possibility of a future change doing an actual, real merge instead of a simple uniquing. However, now that I think about it, such an eventuality (if it ever comes about) would most likely need a separate code path from this anyway.

TL;DR version: You're right. I'll make the changes. :-) Thanks!

-bw