[llvm] r263521 - [MachO] Add MachO alt-entry directive support.

[Lang Hames via llvm-commits]

Hi Rafael,

Apologies - mis-hit the send button. Continuing on...

My patch special-cased 'foo + 0' to behave the same as 'foo' - ld64
recognizes aliases for the same location and treats them as one atom. We
only need to set the alt_entry bit on 'foo + C', where C is non-zero.

As noted in the review this introduces a different bug with regards to
> ld and llvm-mc not agreeing about which symbols are atoms.


This is unfortunate, but the new bug is benign: at worst you get redundant
relocations. With a bit of work in MC I can probably get rid of these too.
The old bug was much more dangerous - you could end up with code that
needed relocations and didn't have any.

- Lang.

On Tue, Mar 22, 2016 at 8:14 AM, Lang Hames <lhames at gmail.com> wrote:

> Hi Rafael,
>
> Why was it decided that the .alt_entry syntax was needed?
>
>
> The majority of the MachO assembly users felt it was clearer syntax. This
> should be MachO specific though - I'll take a look and either change where
> this is parsed, or issue an error if it's parsed for a non-MachO file.
>
> I think this is a odd. You are handling "foo" and "foo + 0" differently.
>
>
> I believe 'foo + 0' is special-cased to behave the same as 'foo
>
> On Tue, Mar 15, 2016 at 10:40 AM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
>
>> I don't think this was ready for commit.
>>
>> Why was it decided that the .alt_entry syntax was needed?
>>
>> > +
>> > +    if (MAI->hasAltEntry() && isa<MCBinaryExpr>(Expr))
>> > +      OutStreamer->EmitSymbolAttribute(Name, MCSA_AltEntry);
>> > +
>>
>> I think this is a odd. You are handling "foo" and "foo + 0"
>> differently. Given what you handle "a = ..." in MC, why do you need to
>> change codegen at all?
>>
>> >    DirectiveKindMap[".symbol_resolver"] = DK_SYMBOL_RESOLVER;
>> > +  DirectiveKindMap[".alt_entry"] = DK_ALT_ENTRY;
>> >    DirectiveKindMap[".private_extern"] = DK_PRIVATE_EXTERN;
>> >    DirectiveKindMap[".reference"] = DK_REFERENCE;
>> >    DirectiveKindMap[".weak_definition"] = DK_WEAK_DEFINITION;
>>
>> Now you get a crash when using .alt_entry on non MachO. It should be
>> an error. That is, you should not be parsing this in generic code,
>> only on MachO specific code.
>>
>> Probably most importantly:
>>
>> As noted in the review this introduces a different bug with regards to
>> ld and llvm-mc not agreeing about which symbols are atoms. Given
>>
>> bar:
>>         .alt_entry foo
>> foo:
>> call bar
>>
>> llvm-mc produces a relocation. Given
>>
>> bar:
>> foo = .
>> call bar
>>
>> we get no relocation. In both cases the linker thinks foo is not an atom.
>>
>> Cheers,
>> Rafael
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160322/a2f1c9af/attachment.html>