[llvm-commits] ARM ELF disassembly with integrated-as

[Greg Fitzgerald]

Tim, thank you for the review.  Comments below:

> First, I'd say that I preferred the refactoring of the previous patch. I
> prefer to keep as much target-specific code as possible in its directory.

When I add the fixes you have suggested, I will also include a second
patch that does nothing but the file refactoring.


> I'm afraid I disagree with ignoring ARM/Thumb switchover too.

I don't disagree with your disagreement. :)  I am hoping to prioritize
fixing
correctness bugs in the assembler.  But since you have provided clear
direction on implementation, I'd be happy to give it a shot.


> You've also forgotten the tests for STT_NOTYPE and STB_LOCAL

I see.  Will fix!


> which tests are the ones that check the duplication code?

There is no test for checking for duplicates.  I don't know how to do that
with FileCheck.  Is there a way or would I need another tool?


> Wouldn't you need two functions to force the Streamer to
> ChangeSection to .text twice?

I don't understand.  Can you clarify?


Thanks,
Greg


On Tue, Nov 13, 2012 at 2:47 AM, Tim Northover <Tim.Northover at arm.com>wrote:

> Hi Greg,
>
> On Friday 09 Nov 2012 22:14:53 Greg Fitzgerald wrote:
> > Compared to the previous patch, this one is small, simple and comes with
> a
> > unit-test.  It doesn’t move any files around and does its best to be a
> > good citizen with regard to coding style.  Could you please review?
>
> First, I'd say that I preferred the refactoring of the previous patch. I
> prefer to keep as much target-specific code as possible in its directory.
>
> I'm afraid I disagree with ignoring ARM/Thumb switchover too. As Jan points
> out, that part of ARM assembly is supported, except that mapping symbols
> won't
> work with it.
>
> As far as I can tell it would be fairly simple to implement. You just need
> to
> also override the EmitAssemblerFlag function in the ARMELFStreamer and do
> the
> right thing for MCAF_Code16 and MCAF_Code32.
>
> You've also forgotten the tests for STT_NOTYPE and STB_LOCAL, and which
> tests
> are the ones that check the duplication code? Wouldn't you need two
> functions
> to force the Streamer to ChangeSection to .text twice?
>
> Other than that, the patch seems reasonable to me. The symbols have the
> correct type and it's valid to omit them for data-only sections so I think
> the
> ABI is satisfied.
>
> Regards.
>
> Tim.
>
>
> _______________________________________________
> 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/20121113/b94a3eac/attachment.html>