DmitryPolukhin created this revision.
DmitryPolukhin added a reviewer: rjmccall.
DmitryPolukhin added a subscriber: cfe-commits.
Fix binary compatibility issue with GCC.
http://reviews.llvm.org/D14980
Files:
lib/AST/RecordLayoutBuilder.cpp
test/Sema/bitfield-layout.c
Index: test/Sema/bitfie
DmitryPolukhin added a comment.
Friendly ping, any comments about this patch?
http://reviews.llvm.org/D14980
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rsmith added a subscriber: rsmith.
rsmith added a comment.
GCC's behavior (`aligned` on a field specifies the alignment of the start of
that field) makes a little more sense to me than Clang's behavior (the type and
alignment of a field specify a flavour of storage unit, and the field goes in
t
rjmccall added a comment.
Well, this is a really nasty bug.
Please check how this interacts with #pragma pack, which normally takes
precedence over even explicit alignment attributes. If that's the case here,
then what you really want to do is just add the same "ExplicitFieldAlign ||"
clause
rjmccall added a comment.
In http://reviews.llvm.org/D14980#298754, @rsmith wrote:
> GCC's behavior (`aligned` on a field specifies the alignment of the start of
> that field) makes a little more sense to me than Clang's behavior (the type
> and alignment of a field specify a flavour of storage
On Mon, Nov 30, 2015 at 11:33 AM, John McCall wrote:
> rjmccall added a comment.
>
> In http://reviews.llvm.org/D14980#298754, @rsmith wrote:
>
> > GCC's behavior (`aligned` on a field specifies the alignment of the
> start of that field) makes a little more sense to me than Clang's behavior
> (t
On Mon, Nov 30, 2015 at 2:31 PM, Richard Smith
wrote:
> On Mon, Nov 30, 2015 at 11:33 AM, John McCall wrote:
>
>> rjmccall added a comment.
>>
>> In http://reviews.llvm.org/D14980#298754, @rsmith wrote:
>>
>> > GCC's behavior (`aligned` on a field specifies the alignment of the
>> start of that
DmitryPolukhin updated this revision to Diff 41481.
DmitryPolukhin added a comment.
Added more testcases to cover combination of explicit alignment of bit-filed
with packed attribute. Also added testing on ARM for bit-filed layout test.
http://reviews.llvm.org/D14980
Files:
lib/AST/RecordLay
DmitryPolukhin added a comment.
I added more testcases and they all pass identically on GCC and clang with my
patch. Please let me know if you think, that some cases are not covered or
doesn't work with my patch. Perhaps we can reduce number of test-cases because
some of them almost duplicates
rjmccall added a comment.
I don't mean the actual layout used by Windows targets; I mean the layout used
by the ms_struct pragma / attribute.
Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ -1605,1 +1605,3 @@
+} else if (ExplicitFieldAlign)
+ FieldOffset = llvm::Ro
DmitryPolukhin updated this revision to Diff 41611.
DmitryPolukhin marked an inline comment as done.
DmitryPolukhin added a comment.
Fixed logic for warning calculation and added even more test-cases.
http://reviews.llvm.org/D14980
Files:
lib/AST/RecordLayoutBuilder.cpp
test/Sema/bitfield-l
DmitryPolukhin added a comment.
This CL doesn't changes anything for ms_struct cases and ms_struct seems to be
broken for bit-fields even for very simple cases so filed separate bug
https://llvm.org/bugs/show_bug.cgi?id=25707
PTAL
Comment at: lib/AST/RecordLayoutBuilder.cpp:1
rjmccall added inline comments.
Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ -1605,1 +1605,3 @@
+} else if (ExplicitFieldAlign)
+ FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
Be sure to test specifically with an APCS A
DmitryPolukhin updated this revision to Diff 41748.
http://reviews.llvm.org/D14980
Files:
lib/AST/RecordLayoutBuilder.cpp
test/Sema/bitfield-layout.c
Index: test/Sema/bitfield-layout.c
===
--- test/Sema/bitfield-layout.c
+++ tes
DmitryPolukhin added inline comments.
Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ -1605,1 +1605,3 @@
+} else if (ExplicitFieldAlign)
+ FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
rjmccall wrote:
> Be sure to test spe
rjmccall added inline comments.
Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ +1605,3 @@
+} else if (ExplicitFieldAlign &&
+ Context.getTargetInfo().useBitFieldTypeAlignment())
+ FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAlign);
DmitryPolukhin added inline comments.
Comment at: lib/AST/RecordLayoutBuilder.cpp:1606
@@ +1605,3 @@
+} else if (ExplicitFieldAlign &&
+ Context.getTargetInfo().useBitFieldTypeAlignment())
+ FieldOffset = llvm::RoundUpToAlignment(FieldOffset, ExplicitFieldAl
DmitryPolukhin updated this revision to Diff 41866.
DmitryPolukhin added a comment.
Added TODO.
http://reviews.llvm.org/D14980
Files:
lib/AST/RecordLayoutBuilder.cpp
test/Sema/bitfield-layout.c
Index: test/Sema/bitfield-layout.c
=
rjmccall added a comment.
Yes, I think that's a reasonable approach.
http://reviews.llvm.org/D14980
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
DmitryPolukhin added a comment.
Added TODO, any other comments or suggestions?
http://reviews.llvm.org/D14980
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
DmitryPolukhin added a comment.
John and Richard,
I think this patch fixes important ABI compatibility issue with GCC and if
there are no more comments, I think it makes sense to commit it. Could you
please approve this CL?
Thanks,
Dmitry
http://reviews.llvm.org/D14980
___
DmitryPolukhin added a comment.
Friendly ping.
I don't think this change makes APCS mode worse. As an alternative I could
return to the variant that doesn't change anything for APCS case. Please let me
know if APCS case must be resolved and TODO is not enough for committing this
change.
http
DmitryPolukhin added a comment.
John and Richard,
I would like to proceed with this patch one way or another. If this patch
cannot be accepted in upstream, I'll discard it. On the other hand I'm ready to
improve this patch further if it is OK in principle but needs more work. Please
let me kno
rjmccall added a comment.
Sorry, holidays. I'm comfortable with taking this patch as-is.
http://reviews.llvm.org/D14980
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL257462: PR18513: make gcc compatible layout for bit-fields
with explicit aligned… (authored by ABataev).
Changed prior to commit:
http://reviews.llvm.org/D14980?vs=41866&id=44609#toc
Repository:
rL L
25 matches
Mail list logo
