[PATCH] Fix alignment of data in TypeLocs (PR16144)

[Eli Friedman]

On Wed, Jun 5, 2013 at 8:34 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Wed, Jun 5, 2013 at 8:06 PM, Eli Friedman <eli.friedman at gmail.com>wrote:
>
>> Patch attached; fixes PR16144.
>>
>> Currently, the data in TypeLocs consists of a bunch of tightly packed
>> structures, and the structures can become misaligned because there isn't
>> any padding.
>>
>> There are basically three possible approaches to fixing the alignment
>> issues in TypeLocs:
>>
>> 1) Force every piece of the TypeLoc's data to have alignment 8.
>> 2) Perform dynamic alignment adjustments.
>> 3) Use #pragma pack to let the compiler know the data is intentionally
>> misaligned.
>>
>> (1) has a substantial impact on memory usage (something like 1% on
>> Cocoa.h), so I'd like to avoid it if possible.  (2) is the attached patch;
>> it avoids both misaligned loads and unnecessary memory usage.  The primary
>> downside is that TypeLocBuilder becomes a lot more complicated, because it
>> doesn't know in advance where it needs to insert padding.  (3) keeps around
>> to misaligned data: there's a potential performance penalty, it requires
>> being careful not to introduce incorrect accesses to the data, and it's
>> just plain ugly.
>>
>>
>> Two questions to focus on for review: would (3) be a better approach?
>>
>
> I've tried this. We expose pointers into the type source info block in a
> couple of places (for instance, the array of ParmVarDecl*s on a
> FunctionTypeLoc) and the misalignment is then exposed to quite a large body
> of code. Maybe a MisalignedArrayRef<...> would help, but the damage is
> still not very contained.
>

Okay, then I'll stick with (2).


> And is there any way to make the TypeLocBuilder implementation a bit less
>> ugly?
>>
>
> Perhaps we could remove the guarantee that the child locations of a
> TypeLoc produced by push<T> are valid, or require some explicit action to
> fix them?
>

A quick run of the regression tests appears to show only one place in the
code where we depend on this, and it's very easy to fix there.  That said,
I'm not really comfortable making this assumption without completely
eliminating temporary TypeLocs from the TypeLocBuilder API: I found another
place that depends on this, but the relevant code pattern didn't show up in
the regression tests.  I'm thinking I'll commit this patch with the current
TypeLocBuilder implementation, and clean up the API in a followup patch.

-Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130606/3644a534/attachment.html>