[PATCH][RFC] Respect alignment of nested bitfields

[Ulrich Weigand]

The attached patch uses the logic you suggest below, this does indeed also
fix the problem I was seeing originally.
Does this look OK?

(See attached file: clang-align-bitfield)


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU/Linux compilers and toolchain
  IBM Deutschland Research & Development GmbH
  Vorsitzende des Aufsichtsrats: Martina Koederitz | Geschäftsführung: Dirk
Wittkopp
  Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht
Stuttgart, HRB 243294



From:	Richard Smith <richard at metafoo.co.uk>
To:	Ulrich Weigand/Germany/IBM at IBMDE, Chandler Carruth
            <chandlerc at google.com>
Cc:	llvm cfe <cfe-commits at cs.uiuc.edu>
Date:	20.04.2015 23:47
Subject:	Re: [PATCH][RFC] Respect alignment of nested bitfields
Sent by:	metafoo at gmail.com



On Mon, Mar 30, 2015 at 11:31 AM, Ulrich Weigand <Ulrich.Weigand at de.ibm.com
> wrote:
  Hello,

  tools/clang/test/CodeGen/packed-nest-unpacked.c contains this test:

  struct XBitfield {
    unsigned b1 : 10;
    unsigned b2 : 12;
    unsigned b3 : 10;
  };
  struct YBitfield {
    char x;
    struct XBitfield y;
  } __attribute((packed));
  struct YBitfield gbitfield;

  unsigned test7() {
    // CHECK: @test7
    // CHECK: load i32, i32* getelementptr inbounds (%struct.YBitfield,
  %struct.YBitfield* @gbitfield, i32 0, i32 1, i32 0), align 4
    return gbitfield.y.b2;
  }

  The "align 4" is actually wrong.  Accessing all of "gbitfield.y" as a
  single
  i32 is of course possible, but that still doesn't make it 4-byte aligned
  as
  it remains packed at offset 1 in the surrounding gbitfield object.

  This alignment was changed by commit r169489, which also introduced
  changes
  to bitfield access code in CGExpr.cpp.  Code before that change used to
  take
  into account *both* the alignment of the field to be accessed within the
  current struct, *and* the alignment of that outer struct itself; this
  logic
  was removed by the above commit.

  This can cause incorrect code to be generated (I've seen an example on
  SystemZ
  where code would crash due to unaligned access).

  The patch below adds back logic to also consider alignment of the outer
  struct
  in EmitLoadOfBitfieldLValue and EmitStoreThroughBitfieldLValue.  It also
  extends the test case to cover the case of storing into the nested
  bitfield.

+  int64_t Align = Info.StorageAlignment;
+  if (!LV.getAlignment().isZero())
+    Align = std::min(Align, LV.getAlignment().getQuantity());

This seems pessimistic. If LV is known to be more aligned than we'd expect
for the bitfield, it seems that we should use its alignment in that case
too. That is:

  int64_t Align = LV.getAlignment().isZero() ? Info.StorageAlignment :
LV.getAlignment().getQuantity();

  (See attached file: clang-align-bitfield)


  Mit freundlichen Gruessen / Best Regards

  Ulrich Weigand

  --
    Dr. Ulrich Weigand | Phone: +49-7031/16-3727
    STSM, GNU/Linux compilers and toolchain
    IBM Deutschland Research & Development GmbH
    Vorsitzende des Aufsichtsrats: Martina Koederitz | Geschäftsführung:
  Dirk
  Wittkopp
    Sitz der Gesellschaft: Böblingen | Registergericht: Amtsgericht
  Stuttgart, HRB 243294
  _______________________________________________
  cfe-commits mailing list
  cfe-commits at cs.uiuc.edu
  http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-align-bitfield
Type: application/octet-stream
Size: 4844 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150421/e355e493/attachment.obj>