<div dir="ltr">Hi David,<div><br></div><div>Ok, here are my main high level comments on the patch. I have some specific nits too but I'll save them for later:</div><div><br></div><div style>#1. I would rather this patch not require modifying the MCAssembler level interface. Instead, what I would like is for the new data structures (IndirectSymbols and IndirectSymbolSections) to get built up inside BindIndirectSymbols().</div>
<div style><br></div><div style>#2. I don't think this patch should actually cause test cases to change. Most of those test cases were generated based on matching existing 'as' output for x86, and the way I envisioned this patch working I wouldn't have expected it to change the order of symbols except in the case where the indirect symbols would be out of order.</div>
<div style><br></div><div style>I believe that if you fix #1 then #2 will fall out, because the indirect symbol lists will get built up in the same order as they are now (based on iterating the indirect symbol list twice).</div>
<div style><br></div><div style> - Daniel</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Mar 14, 2013 at 10:26 AM, David Fang <span dir="ltr"><<a href="mailto:fang@csl.cornell.edu" target="_blank">fang@csl.cornell.edu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Sure, Daniel, you seem pretty active, I can ping again in a week.<br>
<br>
patch notes: The patch is written so that changing a single #if in MCAssembler.h at the top will effectively revert it (for ease of testing). A few typedefs for the new structures in MCAssembler are made public just for convenience.  It looks like I'm deep in FIXME territory, judging by the comments in the touched files.  Feel free to pass a revised patch back to me as you see fit.<br>

<br>
Fang<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
Hi David,<br>
<br>
Thanks for working on a patch, I'll take a look but it may take me a couple<br>
days to get to it.<br>
<br>
- Daniel<br>
<br>
<br>
On Wed, Mar 13, 2013 at 1:48 PM, David Fang <<a href="mailto:fang@csl.cornell.edu" target="_blank">fang@csl.cornell.edu</a>> wrote:<br>
<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
Daniel,<br>
        The resulting LLVM test diffs with my patch are shown here:<br>
</div><a href="http://llvm.org/bugs/show_bug.**cgi?id=14636#c38" target="_blank">http://llvm.org/bugs/show_bug.<u></u>**cgi?id=14636#c38</a><<a href="http://llvm.org/bugs/show_bug.cgi?id=14636#c38" target="_blank">http://<u></u>llvm.org/bugs/show_bug.cgi?id=<u></u>14636#c38</a>><div>
<div class="h5"><br>
<br>
2 tests "failed" in MC/MachO: indirect-symbols.s and symbol-indirect.s<br>
No other LLVM/clang tests regressed (Targets: PPC,X86,ARM).<br>
<br>
The differences are that symbols are re-ordered, which accomplishes the<br>
objective of the patch. There are FIXME comments that require a<br>
maintainer's scrutiny.<br>
Could you review these differences that result from my patch?<br>
The patch as-is now is in proof-of-concept form.  I'd be happy to revise<br>
as you suggest.<br>
If this patch benefits all Mach-O architectures (getting closer to usable<br>
integrated assembly), I'd like to get this into trunk.<br>
<br>
Fang<br>
<br>
<br>
 Michael, Daniel,<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
        The attached patch I wrote works for me so far on my small test<br>
cases: llvm/clang's object file matches the indirect symbol ordering of the<br>
/usr/bin/as-assembled object's (tested on my powerpc-darwin8 branch).<br>
Michael, could you try it out on your own test cases?<br>
Daniel, could you kindly comment and review?  Just the technical merits,<br>
not style.<br>
         I've also attached the same patch to bug 14636.<br>
<br>
Thanks for taking a look.<br>
<br>
Fang<br>
<br>
  Daniel,<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
   Some quick questions about your proposed recipe:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  Ok, here is what I think the right fix is. Instead of creating the<br>
  IndirectSymBase mapping we use to associate sections with their<br>
  indirect offset start, in BindIndirectSymbols() we should:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  1. Add a simple container struct (lets say > ><br>
</blockquote></blockquote></blockquote></blockquote>
MachSectionIndirectSymbols)<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  for tracking the per-section list of indirect symbols. It will keep<br>
  the list of symbols in the section and the index of the first > ><br>
</blockquote></blockquote>
  indirect<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  symbol in the section.<br>
</blockquote></blockquote>
<br>
 Like a map<section*, vector<indirect_symbol> >?<br>
<br>
 Instead of keeping track of the first indirect symbol index, could we<br>
just<br>
 as easily compute the index offsets later during the second pass:<br>
<br>
 offset = 0;<br>
 for each section S (in order)<br>
   record offset for section S<br>
   [optional] write vector of indirect symbols for S<br>
   offset += S.num_indirect_symbols();<br>
 end for<br>
<br>
 I was able to fix the reserve1 field by doing something like this (bug<br>
 14636, comments 30-33), but I only later discovered that the indirect<br>
 symbols needed to be reordered.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  2. Keep a mapping from sections to the above type.<br>
  3. Add a SetVector to record the order of the sections that have<br>
  indirect symbols.<br>
</blockquote></blockquote>
<br>
 This I understand, I found SetVector and SmallSetVector in ADT.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  4. During BindIndirectSymbols() we maintain the above information<br>
  (populating the MachSectionIndirectSymbols per-section symbol > ><br>
</blockquote></blockquote>
  arrays).<br>
<br>
 BindIndirectSymbols looks like it need not change, because it's already<br>
 operating on the assumption of ordered indirect symbols, right?<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  5. Once we have scanned all the symbols we make another pass over<br>
</blockquote></blockquote>
the<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  sections (in the order seen via indirect symbols) and assign the ><br>
</blockquote>
  start<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  indices.<br>
</blockquote></blockquote>
<br>
 as I outlined above (offset = start indices).<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  6. Update writing of the indirect symbol table to write in the same<br>
  order as traversed in #5.<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  Does that make sense? It's more work than your patch but it<br>
</blockquote></blockquote></blockquote></blockquote>
(a) > >   should<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  preserve binary compatibility with 'as' in situations where the<br>
  indirect symbols don't appear out of order w.r.t. the sections,<br>
</blockquote></blockquote>
(b) > >   it<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  makes somewhere more explicit the relationship between sections and<br>
  their list of symbols being in contiguous order.<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  - Daniel<br>
</blockquote></blockquote></blockquote></blockquote>
<br>
 Will this re-ordering be acceptable to all Mach-O back-ends?  x86, ARM,<br>
 PPC?<br>
<br>
 Fang<br>
<br>
<br>
<br>
</blockquote>
<br>
<br>
</blockquote>
--<br>
David Fang<br>
</div></div><a href="http://www.csl.cornell.edu/~**fang/" target="_blank">http://www.csl.cornell.edu/~**<u></u>fang/</a> <<a href="http://www.csl.cornell.edu/~fang/" target="_blank">http://www.csl.cornell.edu/~<u></u>fang/</a>><br>

<br>
<br>
</blockquote>
<br>
</blockquote><div class="HOEnZb"><div class="h5">
<br>
-- <br>
David Fang<br>
<a href="http://www.csl.cornell.edu/~fang/" target="_blank">http://www.csl.cornell.edu/~<u></u>fang/</a><br>
<br>
</div></div></blockquote></div><br></div>