[llvm] r293936 - Change how we handle section symbols on ELF.

[Mikael Holmén via llvm-commits]

Hi,

On 02/10/2017 02:38 PM, Rafael Avila de Espindola wrote:
> Sorry, I missed your first reply. Taking a look at the xcore testcase.

Great! I guessed so.

One could of course argue that there can't be functions on C/C++ level 
starting with "." so the xcore crash can't happen in the real world but 
that would be really sad for us since our section is actually called "code".

Thanks,
Mikael

>
> Cheers,
> Rafael
>
> Mikael Holmén <mikael.holmen at ericsson.com> writes:
>
>> Hi Rafael,
>>
>> Did you have any opinion about this?
>>
>> The small reproducer further down in the mail crashes on xcore.
>>
>> Regards,
>> Mikael
>>
>>
>>  > llc -O0 -march=xcore foo.ll -o -
>>  >
>>  > on
>>  >
>>  > @bar = internal global i32 zeroinitializer
>>  >
>>  > define void @".dp.bss"() {
>>  >   ret void
>>  > }
>>
>>
>> On 02/06/2017 09:37 AM, Mikael Holmén wrote:
>>>
>>>
>>> On 02/03/2017 03:50 PM, Rafael Avila de Espindola wrote:
>>>> Mikael Holmén <mikael.holmen at ericsson.com> writes:
>>>>
>>>>> Hi Rafael,
>>>>>
>>>>> I have a question about this change.
>>>> ...
>>>>>> This patch includes a few hacks to avoid changing our behaviour when
>>>>>> handling conflicts between section symbols and other symbols. I
>>>>>> reported pr31850 to track that.
>>>>>
>>>>> For my out-of-tree backend, we output all code in a section called
>>>>> exactly "code". With your patch we now run into the below problem if we
>>>>> then also have a function called "code".
>>>>>
>>>>> For foo.ll:
>>>>>
>>>>> define void @code() {
>>>>>    ret void
>>>>> }
>>>>>
>>>>> If I do
>>>>>
>>>>> build/bin/llc -O0 -mtriple phoenix foo.ll -o -
>>>>>
>>>>> we get
>>>>>
>>>>>          .section code
>>>>> __FILE_START:
>>>>>          .global code
>>>>> llc: ../lib/MC/MCAsmStreamer.cpp:440: virtual void (anonymous
>>>>> namespace)::MCAsmStreamer::EmitLabel(llvm::MCSymbol *): Assertion
>>>>> `Symbol->isUndefined() && "Cannot define a symbol twice!"' failed.
>>>>>
>>>>> So now we seem to get a clash between the section and function names,
>>>>> and the assertion fails.
>>>>>
>>>>> I suppose this was not intended or are we doing something stupid in our
>>>>> backend?
>>>>
>>>> It was not the intention to change it. What is line 440 for you?
>>>
>>> Ah, sorry.
>>>
>>> It's the assert on the first line in EmitLabel below that blows:
>>>
>>> void MCAsmStreamer::EmitLabel(MCSymbol *Symbol) {
>>>   assert(Symbol->isUndefined() && "Cannot define a symbol twice!");
>>>   MCStreamer::EmitLabel(Symbol);
>>>
>>>   Symbol->print(OS, MAI);
>>>   OS << MAI->getLabelSuffix();
>>>
>>>   EmitEOL();
>>> }
>>>
>>> We have setup our "code" section with this in our *TargetObjectFile.cpp:
>>>
>>>   // .text
>>>   TextSection = Ctx.getELFSection("code", ELF::SHT_LOUSER + 0,
>>>                                   ELF::SHF_ALLOC | ELF::SHF_EXECINSTR);
>>>
>>>> Upstream it is just an empty line.
>>>>
>>>> I tried to reproduce the problem with
>>>>
>>>> define void @".text"() {
>>>>   ret void
>>>> }
>>>>
>>>> but that doesn't crash and the function symbol takes precedence over the
>>>> text one.
>>>
>>> If I do
>>>
>>> llc -O0 -march=xcore foo.ll -o -
>>>
>>> on
>>>
>>> @bar = internal global i32 zeroinitializer
>>>
>>> define void @".dp.bss"() {
>>>   ret void
>>> }
>>>
>>> then I get the same crash for xcore.
>>>
>>> Regards,
>>> Mikael
>>>
>>>>
>>>> Cheers,
>>>> Rafael
>>>>
>>>