[llvm-commits] LLVM, llvm-mc, AsmParser] .weak_reference doesn't allows local names ('L' prefix)

[Jim Grosbach]

On Sep 13, 2011, at 2:01 PM, Stepan Dyatkovskiy wrote:

> Hi Jim,
> 
> .weak_reference symbols are instantiated by OS during runtime only. So I think that's why there will be always error for ".weak_reference L_local" case.
> 

OK. Not a big deal, as you're right, it does make conceptual sense. I'm convinced. :)

> About unconditional diagnostics.
> It seems that using of 'L' prefix is odd for all cases where ParseDirectiveSymbolAttribute is used. But, will be it classified as error for ".global L_local" for example?

Yes. Though that does nothing with the current MC assembler. I.e., the symbol is still considered a temporary and doesn't make it into the object file. As such, it won't break any existing code relying on being able to mark an assembler temp symbol as global.

> And.. As I understood ParseDirectiveSymbolAttribute is used for parsing attributes of any symbols. So hypothetically there may be a case in future where we need to parse attribute for local symbol. So, it seems, that we can't report errors each time when ParseDirectiveSymbolAttribute for local symbols invoked. 

Anything added later that requires this diagnostic to not be emitted can adjust it then. No need to complicate the code now for something that we might want someday.

Committed as r139807.

Thanks for the patch!

Regards,
-Jim

>> On a trivial style note, there's no need for the compound statement braces when there's only one line associated with the 'if' statement like this:
> I fixed it and reattached.
> 
> Regards,
> Stepan.
> 
> 
> 14.09.2011, 00:06, "Jim Grosbach" <grosbach at apple.com>:
>> Hi Stepan,
>> 
>> While it does seem odd to allow those symbols in the directives, is it really an error? Darwin's system assembler doesn't generate diagnostics for any of them when used with an assembler temporary.
>> 
>> $ cat x.s
>> .weak_reference Lfoo
>> .lazy_reference Lfoo
>> .weak_definition Lfoo
>> Lfoo:
>> $ as -arch x86_64 x.s -o x.o
>> $
>> 
>> Assuming we do want a diagnostic, should any of those directives allow assembler local names? That is, does ParseDirectiveSymbolAttribute() really need the new parameter, or should the diagnostic just be unconditional?
>> 
>> On a trivial style note, there's no need for the compound statement braces when there's only one line associated with the 'if' statement like this:
>> +      if (!AllowTemporary && Sym->isTemporary()) {
>> +        return TokError("Local symbol is used in context where only global symbol expected.");
>> +      }
>> 
>> Regards,
>>   Jim
>> 
>> On Sep 13, 2011, at 12:50 PM, Stepan Dyatkovskiy wrote:
>> 
>>>  Hi, everybody.
>>>  I found that next Apple Asm symbol directives should not allow symbols with local names:
>>>  reference
>>>  weak_reference
>>>  lazy_reference
>>>  weak_definition.
>>>  When we parse this ones we should report error.
>>> 
>>>  Please find for review patch that adds error messages for cases described above.
>>> 
>>>  --
>>>  Regards,
>>>  Stepan<undefined_symbols_fix.patch>_______________________________________________
>>>  llvm-commits mailing list
>>>  llvm-commits at cs.uiuc.edu
>>>  http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> <undefined_symbols_fix.patch>