Oops forgot to reply-all originally.  +list now.<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Oct 18, 2012 at 11:46 AM, Greg Fitzgerald <span dir="ltr"><<a href="mailto:gregf@codeaurora.org" target="_blank">gregf@codeaurora.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="">
Jan,<u></u><u></u></p><p class=""><u></u> <u></u></p><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Thanks so much for the review.<u></u><u></u></span></p><div class="im"><p class="">
<u></u> <u></u></p><p class="">> Does this patch handle switching between ARM and thumb code?  E.g., from the llvm test "test/MC/ARM/mode-switch.s"<span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u><u></u></span></p>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p></div><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">No, afraid not.  Is there a concept of switching between ARM and Thumb within LLVM?  As I understand, the triple forces it one way or the other.  Regarding that usage within an assembly file, LLVM currently doesn’t support ARM assembly files targeting ELF, only MachO.  If possible, I’d prefer not to piggyback that feature on this patch, and then address it at the time we add support for parsing ARM ELF assembly (for example, parsing the .eabi_attribute directive).<u></u><u></u></span></p>
<div class="im"><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> </span></p></div></div></div></blockquote><div><br></div><div>I thought there was a concept of switching, based on what "test/MC/ARM/mode-switch.s" does. The CHECKs in the test are the same with both the arm and thumb triples.<br>
</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple">
<div><div class="im"><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u></span></p><p class="">> Do you have a specific test that the mapping symbols really do get emitted, with the right type, binding, etc.?  Perhaps the test should also show that it handles switching between $a, $d, and $t, and squashes duplicates as you had checks for "AlreadySetData", etc.<span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u><u></u></span></p>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p></div><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Not yet.  That’d be a good addition.   And right, a test that also demonstrates why AlreadySetData needs to be there would be good.  As it turns out, only one of those is really needed, and the others are there to claim the functions are idempotent.<u></u><u></u></span></p>
<div class="im"><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p><p class="">> One suggestion: In <span style>2010-11-30-reloc-movt.ll, where the symbols have shifted, it might be more obvious that the change is correct if it had also "OBJ" check for a "Symbol 12", with a "OBJ-NEXT" for foo, or bar, whatever it was looking for, similar to what is done in "elf-reloc-01.ll".</span><u></u><u></u></p>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p></div><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Good tip, I’ll do that.<u></u><u></u></span></p>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Thanks,<u></u><u></u></span></p>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Greg<u></u><u></u></span></p><p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class=""><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p><p class=""><b><span style="font-size:10pt;font-family:Tahoma,sans-serif">From:</span></b><span style="font-size:10pt;font-family:Tahoma,sans-serif"> <a href="mailto:jvoung@google.com" target="_blank">jvoung@google.com</a> [mailto:<a href="mailto:jvoung@google.com" target="_blank">jvoung@google.com</a>] <b>On Behalf Of </b>Jan Voung<br>
<b>Sent:</b> Wednesday, October 17, 2012 10:23 PM<br><b>To:</b> Greg Fitzgerald<br><b>Subject:</b> Re: [llvm-commits] ARM ELF disassembly with integrated-as<u></u><u></u></span></p><div><div class="h5"><p class=""><u></u> <u></u></p>
<div><p class="">Hi Greg,<u></u><u></u></p></div><div><p class=""><u></u> <u></u></p></div><div><p class="">Thanks for working on this!<u></u><u></u></p></div><div><p class=""><u></u> <u></u></p></div><div><p class=""><u></u> <u></u></p>
</div><div><p class="">Two questions:<u></u><u></u></p></div><div><p class="">(1) Does this patch handle switching between ARM and thumb code?  E.g., from the llvm test "test/MC/ARM/mode-switch.s"<u></u><u></u></p>
</div><div><p class=""><u></u> <u></u></p></div><div><p class="">(2) Do you have a specific test that the mapping symbols really do get emitted, with the right type, binding, etc.?  Perhaps the test should also show that it handles switching between $a, $d, and $t, and squashes duplicates as you had checks for "AlreadySetData", etc.<u></u><u></u></p>
</div><div><p class=""><u></u> <u></u></p></div><div><p class=""><u></u> <u></u></p></div><div><p class="">One suggestion: In <span style>2010-11-30-reloc-movt.ll, where the symbols have shifted, it might be more obvious that the change is correct if it had also "OBJ" check for a "Symbol 12", with a "OBJ-NEXT" for foo, or bar, whatever it was looking for, similar to what is done in "elf-reloc-01.ll".</span><u></u><u></u></p>
</div><div><p class=""><u></u> <u></u></p></div><div><p class="">- Jan<u></u><u></u></p></div><div><p class="" style="margin-bottom:12pt"><u></u> <u></u></p><div><p class="">On Wed, Oct 17, 2012 at 8:20 PM, Greg Fitzgerald <<a href="mailto:gregf@codeaurora.org" target="_blank">gregf@codeaurora.org</a>> wrote:<u></u><u></u></p>
<p class="" style="margin-bottom:12pt">> [LLVMdev] R_ARM_ABS32 disassembly with integrated-as<br><br>Please review the attached patch.<br><br>Highlights:<br>* LLVM now emits an ARM ELF with the same mapping symbols as GCC<br>
* Because of that, you can now objdump ARM ELF files and not see gibberish<br>code where jump tables are stored<br>* And having those symbols alleviates concerns Tim mentioned regarding<br>linking.<br><br>Lowlights:<br>* Had the choice of either dumping ARMELFStreamer into the MC layer, or<br>
pulling MCELFStreamer into the public include directory (for MC).  I chose<br>the latter.  It seems quite a bit cleaner, but at the expense of maybe<br>exposing an object that might not be ready to move out of its anonymous<br>
namespace.<br>* Needed to update a few somewhat-unrelated unit tests.  Mostly just<br>updating references in the symbol table to account for the 3 mapping symbols<br>at the start of each file (.text=$a, .data=$d, .bss=$d).  These are probably<br>
worth the most scrutiny.<br><br>Thanks,<br>Greg<br><br>---<br>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted<br>by The Linux Foundation<br><br><br>_______________________________________________<br>
llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><u></u><u></u></p>
</div><p class=""><u></u> <u></u></p></div></div></div></div></div></blockquote></div><br></div>