[llvm-commits] ARM ELF disassembly with integrated-as
Greg Fitzgerald
garious at gmail.com
Tue Nov 13 11:08:34 PST 2012
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>
More information about the llvm-commits
mailing list