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; }
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)>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=67807#toc
Repository:
rL LLVM
https://reviews.llvm.org/D20561
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
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.
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:
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
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, const epoll_event ) {
> out = in;
> }
>
>
>
> Is that as
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
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
;
cfe-commits
Subject: Re: [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
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
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, const epoll_event ) {
out = in;
}
Is that as intended?
Repository:
rL LLVM
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=63970#toc
Repository:
rL LLVM
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
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
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:
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
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.
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 =
It also looks like it will now have false negatives for cases like:
memcpy(x, y, *);
I think whitelisting
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
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
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 <euge...@google.com> wrote:
> On Mon, Jun 13, 2016 at 4:12 PM, Aaron Ballman <aaron.ball...@gmail.com>
> wrote:
>> On Mon, Jun 13,
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
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:
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
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
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=60537#toc
Repository:
rL LLVM
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
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
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.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
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
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
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, "(")
<<
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
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
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
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:
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
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 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
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
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
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
+ //
63 matches
Mail list logo