[PowerPC, Mach-O] PPCMachObjectWriter.cpp, at long last

[David Fang]

Rafael,
 	Thanks for through feedback.  Most of these are easy fixes, but 
before I re-submit the patch, there are a few points to discuss.

>    llvm_unreachable("Relocation emission for MachO/PPC64
> unimplemented, until Fang gets around to hacking...");
>
> report_fatal_error is probably better. We normally don't list devs in
> the source, so just "Relocation emission for MachO/PPC64
> unimplemented".

 	It's hard for me to know in all places whether something is truly 
unreachable or if there is a need to handle a particular switch-case in 
the future.  I've replaced some of the more obvious cases with 
report_fatal_error.  Perhaps PPC maintainers could comment on these in my 
next draft.

> If a user can get here, than report_fatal_error is better. If it is
> something that should not happen, keep the llvm_unrechable, but change
> the message :-)

> +// experimenting with order (endian fishiness on PPC?)
> What does this mean?

I've updated this comment.  The bitfield ordering that is in documentation 
I've seen doesn't match what actually works.  I had to figure this out by 
trial-and-error of object dumps, and comparing against reference output 
from /usr/bin/as, until everything matched.

> What is this test testing for? It should not be testing for every
> assembly line printed by llc. In fact, this patch doesn't change the
> printed assembly, does it? If the patch just changes the output of
> llvm-mc, it should only have an llvm-mc test.

You're right, the asm produced has not been affected by my patch.
Nevertheless, I would like to add some sort of test to make sure -target 
powerpc-apple-darwin8 doesn't break unexpectedly in the future, since it 
has so little coverage in the regression tests.

Also regarding the FileCheck expected output, I would like to use the more 
terse output from otool -ItrvV on the object file, but otool is not 
available outside of darwin, so I'm stuck using llvm's macho-dump.

My next revision of this patch will include only the llvm-mc test.
Would the llc -filetype=asm test be suitable in a separate test-only 
patch?

Fang


-- 
David Fang
http://www.csl.cornell.edu/~fang/