<div dir="ltr">On Tue, Mar 19, 2013 at 2:27 AM, David Fang <span dir="ltr"><<a href="mailto:fang@csl.cornell.edu" target="_blank">fang@csl.cornell.edu</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Daniel,<br>
Thanks for reviewing the patch. I thought I tried suggestion #1 before this patch in <a href="http://llvm.org/bugs/show_bug.cgi?id=14636#c33" target="_blank">http://llvm.org/bugs/show_bug.<u></u>cgi?id=14636#c33</a>, which only reordeed IndirectSymBase without reordering WriteObject, and didn't quite produce the desired result. I may revisit it again.<br>
If I reorder IndirectSymBase, I need to also reorder in WriteObject, so am I allowed to move the IndirectSymbol (mapped by section) and IndirectSymbolSection (order of first appearance) structures out of MCAssembler into MachObjectWriter? I need the ordering information from the first pass of BindIndirectSymbols to persist until WriteObject, without having to sort twice. This transformation is intended for X86,PPC (probably ARM as well) +mach-o back-ends after all.<br>
</blockquote><div><br></div><div style>I don't understand what the issue is here, you can of course add extra data structures into the MachObjectWriter class and compute them during BindIndirectSymbols() and reuse them during WriteObject(). You shouldn't need to change anything in the MCAssembler.</div>
<div style><br></div><div style>Basically I am just suggesting moving the building up of the Mach-O specific data structures into BindIndirectSymbols() and the MachObjectWriter class.</div><div style><br></div><div style>
- Daniel</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Fang<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
Hi David,<br>
<br>
Ok, here are my main high level comments on the patch. I have some specific<br>
nits too but I'll save them for later:<br>
<br>
#1. I would rather this patch not require modifying the MCAssembler level<br>
interface. Instead, what I would like is for the new data structures<br>
(IndirectSymbols and IndirectSymbolSections) to get built up inside<br>
BindIndirectSymbols().<br>
<br>
#2. I don't think this patch should actually cause test cases to change.<br>
Most of those test cases were generated based on matching existing 'as'<br>
output for x86, and the way I envisioned this patch working I wouldn't have<br>
expected it to change the order of symbols except in the case where the<br>
indirect symbols would be out of order.<br>
<br>
I believe that if you fix #1 then #2 will fall out, because the indirect<br>
symbol lists will get built up in the same order as they are now (based on<br>
iterating the indirect symbol list twice).<br>
<br>
- Daniel<br>
<br>
<br>
On Thu, Mar 14, 2013 at 10:26 AM, David Fang <<a href="mailto:fang@csl.cornell.edu" target="_blank">fang@csl.cornell.edu</a>> wrote:<br>
<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
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<br>
MCAssembler.h at the top will effectively revert it (for ease of testing).<br>
A few typedefs for the new structures in MCAssembler are made public just<br>
for convenience. It looks like I'm deep in FIXME territory, judging by the<br>
comments in the touched files. Feel free to pass a revised patch back to<br>
me as you see fit.<br>
<br>
Fang<br>
<br>
Hi David,<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
<br>
Thanks for working on a patch, I'll take a look but it may take me a<br>
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>
Daniel,<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
The resulting LLVM test diffs with my patch are shown here:<br>
</div></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?<u></u>id=14636#c38</a>><br>
<http://**<a href="http://llvm.org/bugs/show_bug.cgi?id=**14636#c38" target="_blank">llvm.org/bugs/show_<u></u>bug.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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
</blockquote>
<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>
<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<br>
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>
<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>
Ok, here is what I think the right fix is. Instead of creating 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">
IndirectSymBase mapping we use to associate sections with their<br>
indirect offset start, in BindIndirectSymbols() we should:<br>
<br>
<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 class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
</blockquote>
MachSectionIndirectSymbols)<br>
</blockquote></blockquote></blockquote>
<br>
<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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
the list of symbols in the section and the index of the first > ><br>
<br>
</blockquote>
indirect<br>
</blockquote>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
symbol in the section.<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
</blockquote>
<br>
</blockquote>
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>
2. Keep a mapping from sections to the above type.<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">
3. Add a SetVector to record the order of the sections that have<br>
indirect symbols.<br>
<br>
</blockquote>
<br>
</blockquote>
This I understand, I found SetVector and SmallSetVector in ADT.<br>
<br>
4. During BindIndirectSymbols() we maintain the above information<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">
(populating the MachSectionIndirectSymbols per-section symbol > ><br>
<br>
</blockquote>
arrays).<br>
</blockquote>
<br>
BindIndirectSymbols looks like it need not change, because it's<br>
already<br>
operating on the assumption of ordered indirect symbols, right?<br>
<br>
5. Once we have scanned all the symbols we make another pass over<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">
<br>
</blockquote>
the<br>
</blockquote>
<br>
<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 class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
</blockquote>
start<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
indices.<br>
<br>
</blockquote>
<br>
</blockquote>
as I outlined above (offset = start indices).<br>
<br>
6. Update writing of the indirect symbol table to write in the same<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">
order as traversed in #5.<br>
<br>
<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 class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
</blockquote>
(a) > > should<br>
</blockquote></blockquote></blockquote>
<br>
<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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
indirect symbols don't appear out of order w.r.t. the sections,<br>
<br>
</blockquote>
(b) > > it<br>
</blockquote>
<br>
<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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
their list of symbols being in contiguous order.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- Daniel<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
</blockquote>
<br>
</blockquote></blockquote></blockquote>
Will this re-ordering be acceptable to all Mach-O back-ends? x86,<br>
ARM,<br>
PPC?<br>
<br>
Fang<br>
<br>
<br>
<br>
<br>
</blockquote>
<br>
--<br>
</blockquote>
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.<u></u>cornell.edu/~**fang/</a>><<br>
<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>
<br>
</blockquote>
<br>
</blockquote><div class="im">
--<br>
David Fang<br>
<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>
</div></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></div>