[patch] Create a MCSymbolELF

[Duncan P. N. Exon Smith]

> On 2015-Jun-01, at 15:56, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
>>> -MCSymbol *MCContext::getOrCreateSectionSymbol(const MCSectionELF &Section) {
>>> -  MCSymbol *&Sym = SectionSymbols[&Section];
>>> +MCSymbolELF *MCContext::getOrCreateSectionSymbol(const MCSectionELF &Section) {
>>> +  MCSymbolELF *&Sym = SectionSymbols[&Section];
>>>   if (Sym)
>>>     return Sym;
>>> 
>>> @@ -128,12 +128,12 @@ MCSymbol *MCContext::getOrCreateSectionSymbol(const MCSectionELF &Section) {
>>> 
>>>   MCSymbol *&OldSym = Symbols[Name];
>>>   if (OldSym && OldSym->isUndefined()) {
>>> -    Sym = OldSym;
>>> -    return OldSym;
>>> +    Sym = cast<MCSymbolELF>(OldSym);
>>> +    return Sym;
>>>   }
>>> 
>>>   auto NameIter = UsedNames.insert(std::make_pair(Name, true)).first;
>>> -  Sym = new (*this) MCSymbol(&*NameIter, /*isTemporary*/ false);
>>> +  Sym = new (*this) MCSymbolELF(&*NameIter, /*isTemporary*/ false);
>> 
>> It'd be nice to `assert(isELF())` at the top of this function, if that's
>> not already there.
> 
> It is already passed a MCSectionELF, so that is statically known.
> 
>>> 
>>>   if (!OldSym)
>>>     OldSym = Sym;
>>> @@ -163,8 +163,14 @@ MCSymbol *MCContext::CreateSymbol(StringRef Name, bool AlwaysAddSuffix) {
>>>   if (AllowTemporaryLabels)
>>>     IsTemporary = Name.startswith(MAI->getPrivateGlobalPrefix());
>>> 
>>> -  if (IsTemporary && AlwaysAddSuffix && !UseNamesOnTempLabels)
>>> -    return new (*this) MCSymbol(nullptr, true);
>>> +  bool IsELF = MOFI && MOFI->getObjectFileType() == MCObjectFileInfo::IsELF;
>> 
>> Should this be in a function?  When is `MOFI` not available?
> 
> MOFI is null in llvm-objdump. Running
> 
> ./bin/llvm-objdump -m -d
> ~/llvm/llvm/test/tools/llvm-objdump/X86/Inputs/exeThread.macho-x86_64
> 
> Fails without this.
> 
>>>   SmallString<128> NewName = Name;
>>>   bool AddSuffix = AlwaysAddSuffix;
>>> @@ -178,7 +184,11 @@ MCSymbol *MCContext::CreateSymbol(StringRef Name, bool AlwaysAddSuffix) {
>>>     if (NameEntry.second) {
>>>       // Ok, we found a name. Have the MCSymbol object itself refer to the copy
>>>       // of the string that is embedded in the UsedNames entry.
>>> -      MCSymbol *Result = new (*this) MCSymbol(&*NameEntry.first, IsTemporary);
>>> +      MCSymbol *Result;
>>> +      if (IsELF)
>>> +        Result = new (*this) MCSymbolELF(&*NameEntry.first, IsTemporary);
>>> +      else
>>> +        Result = new (*this) MCSymbol(false, &*NameEntry.first, IsTemporary);
>> 
>> Actually, this is somewhat awkward code duplication.  It might be better
>> to add a private function like this:
>> 
>>    MCSymbol *MCContext::createSymbolImpl(
>>        const StringMapEntry<...> *Name, bool IsTemporary) {
>>      if (IsELF)
>>        return new (*this) MCSymbolELF(Name, IsTemporary);
>>      return new (*this) MCSymbol(/* IsELF */ false, Name, IsTemporary);
>>    }
> 
> Done. An updated patch is attached.
> 
> Cheers,
> Rafael
> <t.patch>

LGTM.