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

Rafael EspĂ­ndola rafael.espindola at gmail.com
Wed Jul 31 06:41:56 PDT 2013


+ * PPC relocation types from <mach-o/ppc/reloc.h>
+ * (renamed, following conventions in this header)

That file is under a different license, so you cannot just copy and
past from it.


    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".


+       Log2Size is used for relocation_info::r_length.
+       See "include/llvm/MC/MCFixup.h"
+       and "lib/Target/PowerPC/MCTargetDesc/PPCFixupKinds.h"
+       See "lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp":
+               adjustFixupValue() for cases of handling fixups.
+               getFixupKindInfo() for sizes.

Just saying "computes the log2 of the size of the relocation" is
probably better.

+      llvm_unreachable("Unimplemented (relative)");

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 :-)

+// inline

remove this comment.



+// experimenting with order (endian fishiness on PPC?)

What does this mean?

+/**

We normally use /// for comments before a function

+     * odcctools-20090808:as/write_object.c:

please don't copy this.

+#if 0

No commented out code please.

+#if 1

Please remove.

+//      llvm_unreachable("Unhandled PPC scattered relocation type.");

No commented out code.


Please clang format this patch. With git you can just run
clang/tools/clang-format/git-clang-format.

+; This test is paired with test/CodeGen/PowerPC/hello-reloc.s,
+; which tests llvm-mc.

Please remove.

Please remove the use of tee from the run lines.

+; RUN-WORKS-BUT-SKIPPING: llc -filetype=obj -relocation-model=pic
-mcpu=g4 -mtriple=powerpc-apple-darwin8 %s -o - | tee %t2 | macho-dump
| tee %t3 | FileCheck -check-prefix=DARWIN-G4-DUMP %s

Please remove.

+; FIXME: validating .s->.o requires darwin asm syntax support in PPCAsmParser
+; RUN-XFAIL: llvm-mc -relocation-model=pic -mcpu=g4
-triple=powerpc-apple-darwin8 %t1 -o - | tee %t4 | macho-dump | tee
%t5 | FileCheck -check-prefix=DARWIN-G4-DUMP %s
+; RUN-XFAIL: diff -u %t2 %t4 || diff -u %t3 %t5

Please remove. Again, the idea of llvm-mc tests is *not* to feed it
the output of llc. That is a valid way to start creating a test, but
what should be checked in is a reduced testcase.

Please remove all the DARWIN-G4-DUMP lines since they are not being used.

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.


Cheers,
Rafael



More information about the llvm-commits mailing list