[patch] Create a MCSymbolELF
Jim Grosbach
grosbach at apple.com
Mon Jun 1 16:55:16 PDT 2015
Agreed. This is cool. Thanks, Rafael.
-jim
> On Jun 1, 2015, at 4:00 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>
>> 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.
>
More information about the llvm-commits
mailing list