[llvm] r196607 - Integrated assembler incorrectly lexes ARM-style comments

[Renato Golin]

Hi David,

I'd rather not have things like this:

+  /// isARMTarget - special handling for ARM parsing
+  virtual bool isARMTarget() { return false; }

Something more specific to the problem at hand would be better, or even
re-using the infrastructure to determine if the target is ARM, or if it has
@ as comments, or something like that.

cheers,
--renato


On 11 January 2014 02:24, David Peixotto <dpeixott at codeaurora.org> wrote:

> > >> URL: http://llvm.org/viewvc/llvm-project?rev=196607&view=rev
> > >> Log:
> > >> Integrated assembler incorrectly lexes ARM-style comments
> > >>
> > >> The integrated assembler fails to properly lex arm comments when they
> > >> are adjacent to an identifier in the input stream. The reason is that
> > >> the arm comment symbol '@' is also used as symbol variant in other
> > >> assembly languages so when lexing an identifier it allows the '@'
> > >> symbol as part of the identifier.
> > >
> > > This has broken .symver for ELF targets on ARM. Do you plan to fix
> this?
> > > Otherwise I'd ask for a revert.
> >
> > Ugh. Yet another example of why I hate the comment character choice of
> '@'
> > on ARM (no, I don't think we should/can change it; too much history).
> >
> > I agree it's important that symver work as documented. We should:
> >
> > a) fix this bug, it's nasty
> > b) write some better test cases for .symver as we obviously have crappy
> > ones since they're passing even with this change in the tree.
> >
> > David, do you think you'll have a change to have a look at this soon?
>
> I've attached a patch that fixes the issue. I added a test for .symver
> based
> on the one for x86. To get the expected output I first undid my change to
> allow @ in identifiers and then captured the output. I am not familiar with
> what that directive is trying to do, so it would be good if someone could
> sanity-check the test.
>
> Modifying the Lexer to allow @ in the symbol for that one directive was
> relatively easy. The harder part was only doing it for ARM in the
> ELFAsmParser. Any feedback to improve the patch is appreciated.
>
>
>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140111/2842c7c2/attachment.html>