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

[Charles Davis]

On Aug 27, 2013, at 4:59 PM, Nick Kledzik wrote:

> On Aug 26, 2013, at 10:41 PM, Charles Davis <cdavis5x at gmail.com> wrote:
>> On Aug 26, 2013, at 11:32 PM, Charles Davis wrote:
>>> On Aug 26, 2013, at 11:19 PM, David Blaikie wrote:
>>> 
>>>> On Mon, Aug 26, 2013 at 10:00 PM, Charles Davis <cdavis5x at gmail.com> wrote:
>>>>> Author: cdavis
>>>>> Date: Tue Aug 27 00:00:43 2013
>>>>> New Revision: 189315
>>>>> 
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=189315&view=rev
>>>>> Log:
>>>>> Move everything depending on Object/MachOFormat.h over to Support/MachO.h.
>>>> 
>>>> There seem to be a number of unrelated changes in this patch -
>>>> especially for such a large change, please commit the mechanical
>>>> portion separately from any other cleanup/changes.
>>> Sorry. Would you like me to revert this particular change?
>> I went ahead and reverted it (and r189319) in r189321. It looks like it also broke the tests on some platforms (specifically System z and PowerPC--endianness issue? I don't have a big endian machine to test with, so…).
> 
> Chip, 
> 
> 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 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Move-everything-depending-on-Object-MachOFormat.h-ov.patch
Type: application/octet-stream
Size: 164140 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130828/bf97cae5/attachment.obj>