[PATCH] D30129: Fix asm printing of associated sections

[Evgenii Stepanov via llvm-commits]

On Tue, Feb 21, 2017 at 5:34 AM, Rafael Avila de Espindola
<rafael.espindola at gmail.com> wrote:
>> Index: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
>> ===================================================================
>> --- lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
>> +++ lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
>> @@ -1139,7 +1139,8 @@
>>    if (Group)
>>      Flags |= ELF::SHF_GROUP;
>>    MCSectionELF *EHSection = getContext().getELFSection(
>> -      EHSecName, Type, Flags, 0, Group, FnSection.getUniqueID(), &FnSection);
>> +      EHSecName, Type, Flags, 0, Group, FnSection.getUniqueID(),
>> +      dyn_cast<MCSymbolELF>(&Fn));
>
> Can this be a cast?

It can even be a static_cast.

>
>>    assert(EHSection && "Failed to get the required EH section");
>>
>> Index: lib/MC/MCSectionELF.cpp
>> ===================================================================
>> --- lib/MC/MCSectionELF.cpp
>> +++ lib/MC/MCSectionELF.cpp
>> @@ -102,6 +102,8 @@
>>      OS << 'S';
>>    if (Flags & ELF::SHF_TLS)
>>      OS << 'T';
>> +  if (Flags & ELF::SHF_LINK_ORDER)
>> +    OS << 'm';
>>
>>    // If there are target-specific flags, print them.
>>    Triple::ArchType Arch = T.getArch();
>> @@ -152,6 +154,11 @@
>>      OS << ",comdat";
>>    }
>>
>> +  if (Flags & ELF::SHF_LINK_ORDER && AssociatedSymbol) {
>
> Can you assert that there is an AssociatedSymbol if the flag is present?

Done, even though ->getName() call on the next line would have caught
this case pretty reliably.

>
>> Index: include/llvm/MC/MCSectionELF.h
>> ===================================================================
>> --- include/llvm/MC/MCSectionELF.h
>> +++ include/llvm/MC/MCSectionELF.h
>> @@ -45,18 +45,21 @@
>>
>>    const MCSymbolELF *Group;
>>
>> -  /// Depending on the type of the section this is sh_link or sh_info.
>> -  const MCSectionELF *Associated;
>> +  /// sh_link for REL and RELA sections.
>> +  const MCSectionELF *RelInfoSection;
>> +
>> +  /// sh_info for SHF_LINK_ORDER (can be null).
>> +  const MCSymbol *AssociatedSymbol;
>
> Why do you need both? Every section has a symbol associated with it, so
> you could use the section symbol for the relocation section link to the
> relocated section too.

Because the section symbol can be redefined. For example, in

.section x1,"a", at progbits
.local  x1
.comm   x1,4,4

for section "x1", getBeginSymbol()->getSection() points to .bss.
Is that a bug, btw? We could replace getBeginSymbol() with an unnamed
symbol when it is redefined, but then there would be no way to
represent it in textual assembly.

>
> Cheers,
> Rafael