This revision was automatically updated to reflect the committed changes.
Closed by commit rL286798: Remove some false positives when taking the address
of packed members (authored by rogfer01).
Changed prior to commit:
https://reviews.llvm.org/D23657?vs=77498&id=3#toc
Repository:
rL LLV
rogfer01 updated this revision to Diff 77498.
rogfer01 added a comment.
- Address comments.
- Rebase test with current trunk
- Make the check resilient under errors
If no comments arise from this change I will be commiting it Monday next week.
https://reviews.llvm.org/D23657
Files:
include/c
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Some minor changes then this looks good to go.
Comment at: lib/Sema/SemaChecking.cpp:11348-11350
+ // Not the scope of this diagnostic.
+ if (!AnyIsPacked)
+return;
---
joerg added a comment.
It works for me with the mentioned TODO item. But I'm not the right one to give
a full OK.
https://reviews.llvm.org/D23657
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo
rogfer01 added a comment.
Ping? :)
https://reviews.llvm.org/D23657
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 added a comment.
Ping?
https://reviews.llvm.org/D23657
___
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 75395.
rogfer01 added a comment.
Mark comment as TODO
https://reviews.llvm.org/D23657
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
test/Sema/address-packed.c
Index: test/Sema/address-packed.c
===
joerg added a comment.
I agree that leaving out the really complicated parts is acceptable for now.
Can you mark the comment as TODO?
https://reviews.llvm.org/D23657
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-
rogfer01 updated this revision to Diff 75007.
rogfer01 added a comment.
Ignore cases where the innermost base expression is too complicated for the
scope of this patch.
https://reviews.llvm.org/D23657
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
test/Sema/address-packed.c
joerg added a comment.
It seems like on-stack arrays still don't work?
#include
struct test {
uint32_t x;
} __attribute__((__packed__));
int main(void) {
struct test __attribute__((__aligned__(4))) a[4];
uint32_t *p32;
p32 = &a[0].x;
}
https://reviews.llvm.org/D
rogfer01 updated this revision to Diff 74638.
rogfer01 added a comment.
Updated patch, now we check if the innermost base of a MemberExpr is a
DeclRefExpr and check for its declaration in case it provides stronger
alignment guarantees.
https://reviews.llvm.org/D23657
Files:
include/clang/Se
joerg added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:11370
+// we are here such increase has not been enough. So pointing the first
+// FieldDecl that either is packed orelse its RecordDecl is,
+// seems reasonable.
Missing space.
http
rsmith added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:11356-11357
+ // Compute the EffectiveAlignment as the alignment of the whole chain.
+ CharUnits EffectiveAlignment = Context.getTypeAlignInChars(
+ ReverseMemberChain.back()->getParent()->getTypeForDecl()
joerg added a comment.
A simple case that still seems to fail:
#include
struct foo {
uint32_t x;
uint8_t y[2];
uint16_t z;
} __attribute__((__packed__));
typedef struct foo __attribute__((__aligned__(16))) aligned_foo;
int main(void) {
struct foo x;
struct
joerg added a comment.
Seems to work for the false positives I have identified so far.
https://reviews.llvm.org/D23657
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 added a comment.
@arphaman thanks for the testcase! Will do.
https://reviews.llvm.org/D23657
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
arphaman added a comment.
In https://reviews.llvm.org/D23657#560938, @arphaman wrote:
> Btw, slightly off-topic, but I noticed that the declaration
> `DiagnoseMisalignedMembers` in the header has a doc comment that violates the
> 80 chars rule. I committed a fix in r283228.
I meant to say `Di
arphaman added a comment.
Thanks for working on this!
This patch fixes a couple of our internal warnings that shouldn't be presented.
I included a code sample that reproduces our warnings, would you mind adding
some code to the test case that's similar to the sample below?
typedef str
rogfer01 updated this revision to Diff 73466.
rogfer01 added a comment.
Change algorithm following @rsmith suggestions by computing the offset of the
whole access and compare it against the expected alignment, so reduced aligned
structs inside overaligned structs does not yield a warning.
Also
joerg added a comment.
The following is from my comment on the original commit.
I'm trying this patch now. Two instances here show false positives. Some
others are misuse/overuse of __packed, it is time consuming to check all
of them.
- sbin/route
https://nxr.netbsd.org/xref/src/sbin/route/rout
rsmith added inline comments.
> SemaChecking.cpp:11053-11056
> + if ((RequiredAlignment > AlignRecord) ||
> + ((Context.toCharUnitsFromBits(
> +Context.getFieldOffset(cast(MD))) %
> +RequiredAlignment) != 0)) {
This doesn't seem to correctly handle the c
rogfer01 added a comment.
Ping?
https://reviews.llvm.org/D23657
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 added a comment.
Ping?
https://reviews.llvm.org/D23657
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 added a comment.
This is a friendly ping :)
Thank you very much!
https://reviews.llvm.org/D23657
___
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.
https://reviews.llvm.org/D23657
___
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 69968.
rogfer01 added a comment.
Skip the check if the field is already aligned to 1.
https://reviews.llvm.org/D23657
Files:
lib/Sema/SemaChecking.cpp
test/Sema/address-packed.c
Index: test/Sema/address-packed.c
===
rsmith added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:11054
@@ +11053,3 @@
+ auto AlignRecord = Context.getTypeAlignInChars(BaseType);
+ if ((RequiredAlignment > AlignRecord) ||
+ ((Context.toCharUnitsFromBits(
Suppose I have this:
aaron.ballman added a comment.
Some minor nits; @rsmith may have more substantial comments.
Comment at: lib/Sema/SemaChecking.cpp:11039
@@ -11036,2 +11038,3 @@
std::function Action) {
+ // return;
const auto *ME = dyn_cast(E);
Spurious comment.
===
rogfer01 added a comment.
Hi, this is a friendly ping.
Thank you! :)
https://reviews.llvm.org/D23657
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rogfer01 created this revision.
rogfer01 added reviewers: aaron.ballman, rsmith.
rogfer01 added a subscriber: cfe-commits.
This change remove some false positives when taking the address of packed
members.
- It silences the warning when a cast to uintptr_t/intptr_t happens.
- If the field is in
30 matches
Mail list logo
