[Box Backup-dev] PPC workaround (was: COMMIT r436 - in box/trunk: . lib/raidfile)

Ben Summers boxbackup-dev at fluffy.co.uk
Sun Feb 12 12:03:46 GMT 2006


On 12 Feb 2006, at 11:35, Martin Ebourne wrote:

> On Sun, 2006-02-12 at 10:28 +0000, Ben Summers wrote:
>> The ppc hack wasn't necessary on the Mac. gcc -v outputs
>> Should this be specific to a compiler version?
>
> Yes, I guessed you hadn't seen it. The problem is without the facility
> to exhaustively test all versions on the compiler on each platform  
> we've
> no way of implementing blacklisting. On PPC I saw the same failure on
> gcc 3.3.x and 4.0.1, although the workaround for the former didn't  
> work
> for the latter, which is even more scary.
>
> The only other ways are user controlled (my initial preference) and
> writing a proper autoconf check for it. Then implementing a special  
> hack
> for it (like I had before). This is all rather nasty.

And a huge amount of work for a compiler bug.

Is there an easy test case to report to the gcc developers?

>
> This is the reason I took the decision to rewrite that section of code
> in such a way that I could be sure the bug would never be triggered,
> while also trying to improve code readability. The result uses no  
> casts,
> is simple code that the compiler should be very safe with, and to me
> seems easier to follow.

Agreed.

>
>>> Note that there isn't really a call to memcpy, the compiler inlines
>>> it as 2 int copies which is an insignificant overhead (even true
>>> with -O0).
>>
>> You are very trusting of compilers.
>
> I cannot guarantee that all of the time, but there's few compilers  
> left
> which can't make the simple optimisation of a fixed size memcpy. I
> double checked in this case on gcc and it always optimises memcpy  
> away.
>
> Having said that, I don't think it is really essential anyway. The  
> code
> is inside a double nested loop, but only gets called in the final  
> case.
> If I've misunderstood it please say so, but I judged that section  
> to be
> time sensitive, but not time critical.

This is correct.


> Even with a real call to memcpy I
> suspect the overhead would be barely measurable.
>
> I'm happy to revert the previous version and rename the option to
> --enable-bigendian-workaround if you prefer.

I would add

   ASSERT(sizeof(psize) == sizeof(RaidFileRead::FileSizeType));

and a comment about why it's as it is. But I think something which  
definitely works on all platforms is preferable to the user having to  
enable something to make it work.

I'm probably being picky again. A terrible habit.

Ben








More information about the Boxbackup-dev mailing list