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

[Charles Davis]

On Aug 30, 2013, at 12:33 PM, David Fang wrote:

> Chip,
> 	Sure, if it's not too much trouble.  (It simplifies testing, because I have to merge trunk-to-branch, as trunk doesn't build as-is on powerpc-darwin8.)
OK. This patch should be against trunk.

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: 164296 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130830/10bda49f/attachment.obj>
-------------- next part --------------

> 
> Fang
> 
>> 
>> On Aug 30, 2013, at 12:08 PM, David Fang wrote:
>> 
>>> Hi,
>>> 
>>> I had trouble git-applying this latest patch to my tree (master, r189677). Which revision on trunk did you create the patch against?
>> r189543, I think. Would you like an updated patch?
>> 
>> Chip
>> 
>>> If it passed tests for you, and the review looks good, I can also just wait for the commit to land in trunk.
>>> 
>>> Fang
>>> 
>>>> On Aug 28, 2013, at 4:15 PM, David Fang <fang at csl.cornell.edu> wrote:
>>>>> Hi all,
>>>>> 	Did you have more details about the big-endian failures?  Was it by chance hello-reloc.s, which tests powerpc-darwin8?  I contributed the PPCMachObjectWriter, which has some notes about funny endianness in makeRelocationInfo and makeScatteredRelocationInfo.
>>>>> If you have further revisions of this patch that you'd like tested on PowerPC-darwin, I'll be happy to help test.
>>>> 
>>>> Fang,
>>>> 
>>>> Chip posted a revised patch for review earlier today in this thread.  If you could try that out on a PowerPC machine, that would be most helpful.  Thanks.
>>>> 
>>>> -Nick
>>>> 
>>>> 
>>>> 
>>>>>> 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
>>>>>> 
>>>>> 
>>>>> --
>>>>> David Fang
>>>>> http://www.csl.cornell.edu/~fang/
>>>> 
>>> 
>>> --
>>> David Fang
>>> http://www.csl.cornell.edu/~fang/
>>> 
>> 
> 
> -- 
> David Fang
> http://www.csl.cornell.edu/~fang/
>