[llvm] r189315 - Move everything depending on Object/MachOFormat.h over to Support/MachO.h.

[Nick Kledzik]

On Aug 27, 2013, at 11:34 PM, Charles Davis <cdavis5x at gmail.com> wrote:
> On Aug 27, 2013, at 4:59 PM, Nick Kledzik wrote:
>> After looking at the failures, the problematic area is with defining relocation_info using bit fields.  Your patch uses:
>> 
>> +    struct scattered_relocation_info {
>> +#if defined(BYTE_ORDER) && defined(BIG_ENDIAN) && (BYTE_ORDER == BIG_ENDIAN)
>> +      uint32_t r_scattered:1,
>> +               r_pcrel:1,
>> +               r_length:2,
>> +               r_type:4,
>> +               r_address:24;
>> +#else
>> +      uint32_t r_address:24,
>> +               r_type:4,
>> +               r_length:2,
>> +               r_pcrel:1,
>> +               r_scattered:1;
>> +#endif
>> +      int32_t r_value;
>> +    };
>> 
>> 
>> But whether all three of those preprocessor symbols (e.g. BIG_ENDIAN) are set up is hazy.  My guess is one of the is not set up on the failing big endian builders and causing the relocations to be messed up.
> Really? I thought it was because the other struct (relocation_info) *doesn't* use them. So now the order of the bitfields depends on the host endianness--which, when we're making an object file, means that the correct order also depends on the *target* endianness. Unfortunately, we're stuck with this, because the definition was cribbed from <mach-o/reloc.h>, and it has the same problem. (The definition of scattered_relocation_info was also cribbed from <mach-o/reloc.h>; that's why I used the endianness macros there. In fact, I believe I originally used Apple's __BIG_ENDIAN__ macro for this, but I changed it because I figured this wouldn't get defined everywhere it needed to be.) As far as I could tell, the relocations that came out wrong on those machines were plain relocations, not scattered relocations. In particular, several x86_64 relocations came out wrong, and x86_64 doesn't use scattered relocations at all.
I’ve finished reviewing your new patch.  It all looks good.  In particular, it now uses the word0/word1 technique for all relocations instead of using bit fields.   That technique is what llvm used before to avoid bit field issues.  

-Nick

>> I don’t see many of the endian conditionals used anywhere in LLVM.  I’d suggest keeping the MC stuff using the word0 and word1 style of access and avoid using bitfields.
> I figured you'd say that, and it sounds like a good idea regardless: it worked before, after all ;). (It just took me some time to audit my change and make sure there was nothing beyond mechanical renaming so nothing else like this happens.)
> 
> New patch attached. In addition to continuing to use MachO::any_relocation_info, it also incorporates David's warning fix from r189319. I don't want to cause any more churn than I already have, so I won't commit until someone goes over it and gives me their LG.
> 
> Chip
> <0001-Move-everything-depending-on-Object-MachOFormat.h-ov.patch>