This revision was automatically updated to reflect the committed changes.
Closed by commit rGf7b9f3149b76: Add missing tests (authored by rogfer01).
Herald added a project: clang.
Changed prior to commit:
https://reviews.llvm.org/D20561?vs=67807&id=223564#toc
Repository:
rG LLVM Github Monore
leonardchan added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
I think I found a false negative with this where if the member is accessed from
a packed struct type returned from a function, the warning does not appear:
typedef struct {
uint8_t a;
ui
joerg added a comment.
Yes, the project is interested on reducing the number of false positives. The
example you gave is *not* a FP, but exactly the kind of situation the warning
is supposed to trigger on.
Repository:
rL LLVM
https://reviews.llvm.org/D20561
__
royger added a comment.
Ping?
It's not clear to me whether upstream is going to do something about this or
not. I would like to know in case I need to start passing
"-Waddress-of-packed-member" around.
Repository:
rL LLVM
https://reviews.llvm.org/D20561
_
joerg added a comment.
The last review left out the case of naturally aligned packed members, IIRC. I
have to go over the warning list in NetBSD again, but I'm moderately sure it is
not fixed.
The better example is:
struct __attribute__((__packed__)) bar {
uint16_t x1;
uint16_t x
royger added a comment.
In https://reviews.llvm.org/D20561#663069, @joerg wrote:
> @royger: Your example is missing explicit alignment. packed has two side
> effects: remove internal padding and set the alignment to 1. That means that
> the offset of base doesn't matter so much because reg itse
rogfer01 added a comment.
In https://reviews.llvm.org/D20561#663069, @joerg wrote:
> It is not true that all false positives have been fixed, some of the more
> complex cases involving nested data structures are still open.
I could not find any in bugzilla after a quick search. So please feel
joerg added a comment.
It is not true that all false positives have been fixed, some of the more
complex cases involving nested data structures are still open.
@royger: Your example is missing explicit alignment. packed has two side
effects: remove internal padding and set the alignment to 1. T
royger added a comment.
In https://reviews.llvm.org/D20561#663006, @kimgr wrote:
> ... and both revisions should be in the 4.0 branch (taken from r291814)
>
> I was looking forward to this warning to help iron out alignment issues at
> compile-time instead of runtime (our ARM CPUs don't like una
kimgr added a comment.
... and both revisions should be in the 4.0 branch (taken from r291814)
I was looking forward to this warning to help iron out alignment issues at
compile-time instead of runtime (our ARM CPUs don't like unaligned access).
@royger, can you expand a little on what you mean
rogfer01 added a comment.
In https://reviews.llvm.org/D20561#662963, @royger wrote:
> In https://reviews.llvm.org/D20561#662959, @rogfer01 wrote:
>
> > We fixed all identified false positives in later patches to this one. So
> > maybe you want to check against trunk clang. If trunk still diagnos
royger added a comment.
In https://reviews.llvm.org/D20561#662959, @rogfer01 wrote:
> We fixed all identified false positives in later patches to this one. So
> maybe you want to check against trunk clang. If trunk still diagnoses false
> positives, please report them to me and I will be more t
rogfer01 added a comment.
In https://reviews.llvm.org/D20561#662922, @royger wrote:
>
Hi,
some targets do not support unaligned accesses so this warning is valuable in
terms of portability. In platforms where unaligned accesses are allowed, it may
happen that such accesses incur in a perform
royger reopened this revision.
royger added a comment.
This revision is now accepted and ready to land.
Hello,
This addition is silly, and too noisy for real world usage.
For once, it doesn't really check whether the access is actually unaligned or
not, and then on x86 for example unaligned acc
On Mon, Aug 15, 2016 at 6:20 PM, Matthias Braun wrote:
> MatzeB added a subscriber: MatzeB.
> MatzeB added a comment.
>
> The sanitizer code triggers this warning for code that looks conceptually
> like this:
>
> typedef struct Bla { char bar; int foo; } __attribute__((packed));
>
> uintptr_t
MatzeB added a subscriber: MatzeB.
MatzeB added a comment.
The sanitizer code triggers this warning for code that looks conceptually like
this:
typedef struct Bla { char bar; int foo; } __attribute__((packed));
uintptr_t getu(struct Bla *b) { return (uintptr_t)&b->foo; }
Resulting in:
This revision was automatically updated to reflect the committed changes.
Closed by commit rL278483: This patch implements PR#22821. (authored by
rogfer01).
Changed prior to commit:
https://reviews.llvm.org/D20561?vs=64114&id=67807#toc
Repository:
rL LLVM
https://reviews.llvm.org/D20561
Fi
rogfer01 added a comment.
Hi, friendly ping.
@rsmith any further comment on this?
Thank you very much.
https://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi
rogfer01 added a comment.
> Please wait until someone has had the chance to review before committing (the
> fix does not appear trivial and the original code broke code). I'm on
> vacation today (in theory), but perhaps @rsmith will have a chance to review.
Sure. Thanks.
https://reviews.llvm
aaron.ballman added a comment.
In https://reviews.llvm.org/D20561#486994, @rogfer01 wrote:
> I plan to commit this today. It only diagnoses address-taking of packed
> fields.
Please wait until someone has had the chance to review before committing (the
fix does not appear trivial and the orig
rogfer01 added a comment.
I plan to commit this today. It only diagnoses address-taking of packed fields.
Reference binding to packed fields will be addressed in another change as
proper support may involve codegen changes which are out of the scope of this
work.
Any further comments are welco
rogfer01 updated the summary for this revision.
rogfer01 removed rL LLVM as the repository for this revision.
rogfer01 updated this revision to Diff 64114.
rogfer01 added a comment.
Remove the diagnostic for the binding of references.
https://reviews.llvm.org/D20561
Files:
include/clang/Basic
rogfer01 added a comment.
I've opened https://llvm.org/bugs/show_bug.cgi?id=28571 to track the reference
binding issue.
Repository:
rL LLVM
https://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm
rogfer01 added a comment.
In https://reviews.llvm.org/D20561#484421, @jyknight wrote:
> This seems to trigger even for the implicitly generated copier of a packed
> struct. E.g.
>
> #include
>
> void copyit(epoll_event&out, const epoll_event &in) {
> out = in;
> }
>
>
>
> Is that
rogfer01 reopened this revision.
rogfer01 added a comment.
This revision is now accepted and ready to land.
I suggest that we handle the reference binding diagnostic in another change and
leave this one only for the address taking diagnostic.
Repository:
rL LLVM
https://reviews.llvm.org/D205
rogfer01 added a comment.
Reverted.
Repository:
rL LLVM
https://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
: [PATCH] D20561: Warn when taking address of packed member
Roger, can you please revert? This seems to have caused some pain for
our users, and it would be good to unblock them while deciding what to
do about the issues.
~Aaron
On Thu, Jul 14, 2016 at 2:19 PM, James Y Knight wrote:
> jyknight adde
Roger, can you please revert? This seems to have caused some pain for
our users, and it would be good to unblock them while deciding what to
do about the issues.
~Aaron
On Thu, Jul 14, 2016 at 2:19 PM, James Y Knight wrote:
> jyknight added a comment.
>
> Regardless, I think this should not have
jyknight added a comment.
Regardless, I think this should not have added a new un-disableable error, but
instead only a default-on warning.
The ""binding reference to packed member" error is firing on some of our code,
and even if it's not a false-positive, it should be possible to disable it
jyknight added a subscriber: jyknight.
jyknight added a comment.
This seems to trigger even for the implicitly generated copier of a packed
struct. E.g.
#include
void copyit(epoll_event&out, const epoll_event &in) {
out = in;
}
Is that as intended?
Repository:
rL LLVM
https:/
This revision was automatically updated to reflect the committed changes.
Closed by commit rL275417: Diagnose taking address and reference binding of
packed members (authored by rogfer01).
Changed prior to commit:
https://reviews.llvm.org/D20561?vs=61089&id=63970#toc
Repository:
rL LLVM
htt
aaron.ballman added a comment.
In http://reviews.llvm.org/D20561#482963, @rogfer01 wrote:
> If there are not any objections I will commit this tomorrow.
If @rsmith doesn't comment by tomorrow, then I think any feedback can be
handled post-commit instead.
http://reviews.llvm.org/D20561
___
rogfer01 added a comment.
If there are not any objections I will commit this tomorrow.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 added a comment.
Ping?
Looking forward any further comments before committing.
Thank you very much.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
rogfer01 added a comment.
Ping?
Any comment on this?
Thank you very much.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman added a comment.
In http://reviews.llvm.org/D20561#466545, @rogfer01 wrote:
> Ping? Any further comments, thoughts?
>
> Thank you very much.
The usual practice is to ping after a week has gone by, btw. There were
standards meetings this week, so that may be delaying some of the c
rogfer01 added a comment.
Ping? Any further comments, thoughts?
Thank you very much.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 added a comment.
Ping?
Thank you very much.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 updated this revision to Diff 61089.
rogfer01 added a comment.
Following @rsmith suggestion, the diagnostic is silenced if the address is
converted to a pointer with lower or equal alignment requirements.
http://reviews.llvm.org/D20561
Files:
include/clang/Basic/DiagnosticSemaKinds.
rogfer01 marked an inline comment as done.
rogfer01 added a comment.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rsmith added a comment.
I agree, that looks like correct behavior for the warning. Thanks!
Comment at: lib/Sema/SemaCast.cpp:259-260
@@ -258,2 +258,4 @@
return ExprError();
+ if (DestType->isVoidPointerType())
+DiscardMisalignedMemberAddress(E);
}
eugenis added a comment.
This timeval thing looks like a legitimate warning to me.
I don't think the analysis should go beyond the function boundaries. If a
callee expects timeval * as part of its signature it should get a properly
aligned timeval *.
http://reviews.llvm.org/D20561
_
rogfer01 added a comment.
Firefox build shows a couple of warnings in the sctp library that already gave
warnings with the earlier patches.
That code has the following structure.
#define SCTP_PACKED __attribute__((packed))
#define SCTP_IDENTIFICATION_SIZE 16
// ...
/* state cookie heade
rogfer01 updated the summary for this revision.
rogfer01 updated this revision to Diff 60951.
rogfer01 added a comment.
Thanks @rsmith for the suggestions. I removed the whitelisting totally and
changed the strategy. This patch implements your second suggestion which seemed
more obvious to me. S
rsmith added a comment.
This still looks like it will have has lots of false positives for cases like:
struct __attribute__((packed)) A {
char c;
int n;
} a;
void *p = &a.n;
It also looks like it will now have false negatives for cases like:
memcpy(x, y, *&a.n);
I think whiteli
rogfer01 added a comment.
A build of Firefox does not show any false positive now.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 removed rL LLVM as the repository for this revision.
rogfer01 updated this revision to Diff 60664.
rogfer01 added a comment.
This new patch adds a whitelist for memcpy, memmove, memset and memcmp.
I'm not entirely happy with the strategy that I'm using, where Parser
communicates to Sema
rogfer01 reopened this revision.
rogfer01 added a comment.
This revision is now accepted and ready to land.
I've reverted the commit.
During implementation of the white list I felt the changes were too much for a
follow up commit, so I prefer some feedback first.
Repository:
rL LLVM
http://
chard Smith; cfe-commits
Subject: Re: [PATCH] D20561: Warn when taking address of packed member
On Mon, Jun 13, 2016 at 7:17 PM, Evgenii Stepanov wrote:
> On Mon, Jun 13, 2016 at 4:12 PM, Aaron Ballman
> wrote:
>> On Mon, Jun 13, 2016 at 6:30 PM, Evgeniy Stepanov wrote:
&g
On Mon, Jun 13, 2016 at 7:17 PM, Evgenii Stepanov wrote:
> On Mon, Jun 13, 2016 at 4:12 PM, Aaron Ballman
> wrote:
>> On Mon, Jun 13, 2016 at 6:30 PM, Evgeniy Stepanov wrote:
>>> eugenis added a subscriber: eugenis.
>>> eugenis added a comment.
>>>
>>> In http://reviews.llvm.org/D20561#446031,
On Mon, Jun 13, 2016 at 4:12 PM, Aaron Ballman wrote:
> On Mon, Jun 13, 2016 at 6:30 PM, Evgeniy Stepanov wrote:
>> eugenis added a subscriber: eugenis.
>> eugenis added a comment.
>>
>> In http://reviews.llvm.org/D20561#446031, @aaron.ballman wrote:
>>
>>> In http://reviews.llvm.org/D20561#44582
On Mon, Jun 13, 2016 at 6:30 PM, Evgeniy Stepanov wrote:
> eugenis added a subscriber: eugenis.
> eugenis added a comment.
>
> In http://reviews.llvm.org/D20561#446031, @aaron.ballman wrote:
>
>> In http://reviews.llvm.org/D20561#445824, @rogfer01 wrote:
>>
>> > I think I wasn't clear with the pur
eugenis added a subscriber: eugenis.
eugenis added a comment.
In http://reviews.llvm.org/D20561#446031, @aaron.ballman wrote:
> In http://reviews.llvm.org/D20561#445824, @rogfer01 wrote:
>
> > I think I wasn't clear with the purpose of the fix-it: there are a few
> > cases where getting the addr
This revision was automatically updated to reflect the committed changes.
Closed by commit rL272552: Warn when taking address of a packed member
(authored by rogfer01).
Changed prior to commit:
http://reviews.llvm.org/D20561?vs=59342&id=60537#toc
Repository:
rL LLVM
http://reviews.llvm.org/
rogfer01 added a comment.
Thank you Aaron. I will commit ASAP.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman added a comment.
I think Richard has had enough time to comment. You can go ahead and commit,
and if there are any issues, they can be handled post-commit. Thanks!
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commit
rogfer01 added a comment.
Ping?
Thanks a lot.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 marked an inline comment as done.
rogfer01 added a comment.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
I think the patch LGTM (with a minor formatting nit). @rsmith, what are your
thoughts?
Comment at: test/SemaCXX/address-packed.cpp:92
@@ +91,3 @@
+void g1()
+{
rogfer01 updated this revision to Diff 59342.
rogfer01 marked 3 inline comments as done.
rogfer01 added a comment.
Remove fix-it as it is not suitable for this diagnostic.
http://reviews.llvm.org/D20561
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/addr
rogfer01 marked 3 inline comments as done.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5392
@@ -5388,1 +5391,3 @@
+def note_address_of_packed_member_silence : Note<
+ "place parentheses around the '%0' expression to silence this warning">;
aaron.ball
aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
In http://reviews.llvm.org/D20561#445824, @rogfer01 wrote:
> I think I wasn't clear with the purpose of the fix-it: there are a few cases
> where getting the address of
rogfer01 updated this revision to Diff 59241.
rogfer01 added a comment.
This change adds a fix-it suggesting parentheses to silence the warning.
http://reviews.llvm.org/D20561
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/address-packed.c
test/SemaCXX
rogfer01 marked 5 inline comments as done.
rogfer01 added a comment.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 added a comment.
I think I wasn't clear with the purpose of the fix-it: there are a few cases
where getting the address of an unaligned pointer is safe (i.e. false
positives).
For instance, when I checked Firefox and Chromium there are cases where getting
the address of an unaligned p
rogfer01 marked 2 inline comments as done.
rogfer01 added a comment.
Well, the assignment-vs-comparison warning does emit a fix-it in
`Sema::DiagnoseAssignmentAsCondition(Expr *E)`
Diag(Loc, diag::note_condition_assign_silence)
<< FixItHint::CreateInsertion(Open, "(")
<< FixItH
aaron.ballman added a comment.
In http://reviews.llvm.org/D20561#445730, @rogfer01 wrote:
> It came to my mind that might be good idea adding one of those "fix-it"
> suggestions so the user knows it can silence the warning by using
> parentheses. What do you think?
I don't think that would be
rogfer01 added a comment.
It came to my mind that might be good idea adding one of those "fix-it"
suggestions so the user knows it can silence the warning by using parentheses.
What do you think?
http://reviews.llvm.org/D20561
___
cfe-commits mail
aaron.ballman added a comment.
This is getting really close, I mostly only have nits left.
Comment at: lib/Sema/SemaExpr.cpp:10529
@@ +10528,3 @@
+bool ByteAligned = Context.getTypeAlignInChars(MD->getType()).isOne();
+if (ByteAligned) // packed has had not any effect on
rogfer01 marked an inline comment as done.
rogfer01 added a comment.
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 updated this revision to Diff 59178.
rogfer01 marked an inline comment as done.
rogfer01 added a comment.
Do not warn if the alignment of the type of the field is already 1 as packed
does not have any effect on those fields.
http://reviews.llvm.org/D20561
Files:
include/clang/Basic/
aaron.ballman added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:10527
@@ +10526,3 @@
+if (RD->hasAttr() ||
+ME->getMemberDecl()->hasAttr()) {
+ Diag(OpLoc, diag::warn_taking_address_of_packed_member)
rogfer01 wrote:
> aaron.ballman wrote:
>
rogfer01 marked 3 inline comments as done.
Comment at: lib/Sema/SemaExpr.cpp:10527
@@ +10526,3 @@
+if (RD->hasAttr() ||
+ME->getMemberDecl()->hasAttr()) {
+ Diag(OpLoc, diag::warn_taking_address_of_packed_member)
aaron.ballman wrote:
> Ah, I forgo
aaron.ballman added a comment.
In http://reviews.llvm.org/D20561#442051, @rogfer01 wrote:
> Only warn if the expression is of the form &a.x this way &(a.x) can be used
> to silence the warning.
I think this is a reasonable idea, but would still like to make sure we have a
low false-positive r
rogfer01 added a comment.
Ping?
Thank you very much
http://reviews.llvm.org/D20561
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 updated this revision to Diff 58760.
rogfer01 added a comment.
Only warn if the expression is of the form &a.x this way &(a.x) can be used to
silence the warning.
http://reviews.llvm.org/D20561
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/add
rogfer01 added a comment.
Forget my wrong comment about packed fields that might actually be aligned. It
does not make sense.
Regarding the way to selectively disable this, there is little syntax available
at this point that we can leverage, so I propose to use parentheses as a way to
disable
rogfer01 added a comment.
Firefox build has ended and exposes 62 diagnostics, 44 unique ocurrences and 10
different diagnostics (shown below) in networking code.
taking address of packed member 'address' of class or structure
'sctp_state_cookie' may result in an unaligned pointer value
taki
rogfer01 marked 4 inline comments as done.
Comment at: lib/Sema/SemaExpr.cpp:10527
@@ +10526,3 @@
+ME->getMemberDecl()->hasAttr()) {
+ Diag(OpLoc, diag::warn_taking_address_of_packed_member)
+ << ME->getMemberDecl() << RD;
aaron.ballman wrote
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a comment.
Thank you for working on this diagnostic! A few questions and comments below.
Comment at: lib/Sema/SemaExpr.cpp:10518
@@ +10517,3 @@
+ // Taking the address of a data member/field of a packed
+ //
rogfer01 created this revision.
rogfer01 added a reviewer: rsmith.
rogfer01 added a subscriber: cfe-commits.
This patch implements PR#22821.
Taking the address of a packed member is dangerous since the reduced
alignment of the pointee is lost. This can lead to memory alignment
faults in some arch
81 matches
Mail list logo
