[LLVMdev] please review this fix for PR3510

[Stuart Hastings]

Please review this patch for PR3510 (and <rdar://problem/6564697>).   
The bug is a failure to handle a "hole" inside an initialized  
structure, where the hole may be induced by a designated initializer  
or by alignment:

	http://llvm.org/bugs/show_bug.cgi?id=3510

The original code was greatly simplified by using FieldNo to index the  
LLVM fields and the initializer in lock-step.  Alas, that fails when  
the initializer requires alignment padding; such padding is not  
recorded in the LLVM structure type.  This implies that the  
initialized ResultElts[] may have many more fields than the LLVM type.

The patched code tracks the HighWaterMarkInBits; it points to the next  
bit to allocate.  If the starting offset of the next initializing  
value doesn't match, either byte-alignment or padding is indicated.   
FieldNo counts the fields (initializers or padding) created in  
ResultElts[], and LLVMFieldNo walks through the LLVM structure type  
fields.  Note that ResultElts[] is now created to hold 2X the number  
of LLVM fields in order to accommodate padding fields; it is shrunk-to- 
fit after the initializer is complete.

I've seen hints in the code that C++ can generate overlapping field  
declarations, but I haven't personally seen this, and my patch has no  
explicit provision for it.  If any reader has experience with this  
issue, I would be grateful for assistance or a demonstrating  
testcase.  I would not be surprised if my patch failed when  
encountering an unholy convergence of packed designated bitfields or  
whatever.

The existing ProcessBitFieldInitialization() is invoked for every  
bitfield; it expects to be passed the same FieldNo pointing at a  
previously-created initializer so it can modify a previously-existing  
constant value.  This is counter-intuitive when compared with the non- 
bitfield case that increments FieldNo after every initialization is  
processed, but I believe this is necessary.  However, there is an  
awkward mental gear-shift when encountering a non-bitfield initializer  
following a bitfield; this is handled with an ugly  
"PredecessorWasBitfield" state-variable.  (Suggestions for a less- 
inelegant solution are welcome.)

The patch handles the PR3510 testcase correctly and has successfully  
passed the GCC DejaGNU testsuite.  However, I don't think we have  
sufficient tests for this issue, so I'm working on a few new testcases  
in the background.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: llvm-gcc.test.diffs.txt
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090219/f3d97bb6/attachment.txt>
-------------- next part --------------



stuart