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

[Rafael Espíndola]

On 1 August 2013 16:00, David Fang <fang at csl.cornell.edu> wrote:
> Hi,
>         Attached is another revision of the patch.  I think I've addressed
> your points.  Does this look better to you?

I am getting warnings like

PPCMachObjectWriter.cpp:101:10: warning: case value not in enumerated
type 'llvm::MCFixupKind' [-Wswitch]
    case PPC::fixup_ppc_brcond14:

I guess you should do the same as the ELF writer:

switch ((unsigned)Fixup.getKind()) {

+  enum RelocationInfoTypePPC {
+    RIT_PPC_VANILLA, /* generic relocation as discribed above */
+    RIT_PPC_PAIR,    /* the second relocation entry of a pair */

You are still copy and pasting from a system header. Don't do that.
Please just use the existing RIT_Vanilla and RIT_Pair and start the
ppc relocations from 3.

+
 using namespace llvm;

unnecessary white space change.

+  // FIXME: see <mach-o/reloc.h> for note on endianness

The online manual is probably a better reference.


+; I took the asm produced by llc -filetype=asm hello-reloc.ll and

The comment is out of date. It should probably be something on the lines of:

This test uses ELF syntax since asm parser support for the MachO
syntax has not been implemented yet. I should be update when that is
available.

Please document what the test is testing. For example, is it checking
which relocations get produced for a branch to an external symbol? Can
you try llvm-objdump? It should produce easier to read/check output.

And yes, the new patch is much better. Thanks.

> Fang

Cheers,
Rafael