[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
fhahn wrote: Thanks! https://github.com/llvm/llvm-project/pull/82922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
https://github.com/fhahn closed https://github.com/llvm/llvm-project/pull/82922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
https://github.com/efriedma-quic approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/82922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
fhahn wrote: > Whitespace is weird in a few places; otherwise looks fine. Argh yes, should be fixed in the latest version. Still need to figure out how to make sure all changes are formatted once the branch contains merges (before the format checker complains and shows the commits) https://github.com/llvm/llvm-project/pull/82922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
https://github.com/fhahn updated https://github.com/llvm/llvm-project/pull/82922
>From 556fcefed9645aa0a0a965ff8f22d8accdf9eefc Mon Sep 17 00:00:00 2001
From: Florian Hahn
Date: Sun, 25 Feb 2024 13:23:17 +
Subject: [PATCH 1/7] [TBAA] Skip all bitfields when generating !tbaa.struct
metadata.
At the moment, clang generates what I believe are incorrect !tbaa.struct
fields for named bitfields. At the moment, the base type size is used
for named bifields (e.g. sizeof(int)) instead of the bifield width per
field. This results in overalpping fields in !tbaa.struct metadata.
This causes incorrect results when extracting individual copied fields
from !tbaa.struct as in added in dc85719d5.
This patch fixes that by skipping all bitfields, not only unnamed ones
(note that CollectFields has a TODO to support bitfields). As bitfields
specify their widths in bits, while !tbaa metadata uses bytes for sizes
and offsets, I don't think we would be able to generate correct metadata
for them in general.
If this understanding is correct, I can also extend the verifier to
check that !tbaa.struct fields aren't overlapping.
Fixes https://github.com/llvm/llvm-project/issues/82586
---
clang/lib/CodeGen/CodeGenTBAA.cpp | 2 +-
clang/test/CodeGen/tbaa-struct.cpp | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp
b/clang/lib/CodeGen/CodeGenTBAA.cpp
index dc288bc3f6157a..43a1aee3d73823 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -298,7 +298,7 @@ CodeGenTBAA::CollectFields(uint64_t BaseOffset,
unsigned idx = 0;
for (RecordDecl::field_iterator i = RD->field_begin(),
e = RD->field_end(); i != e; ++i, ++idx) {
- if ((*i)->isZeroSize(Context) || (*i)->isUnnamedBitfield())
+ if ((*i)->isZeroSize(Context) || (*i)->isBitField())
continue;
uint64_t Offset = BaseOffset +
Layout.getFieldOffset(idx) / Context.getCharWidth();
diff --git a/clang/test/CodeGen/tbaa-struct.cpp
b/clang/test/CodeGen/tbaa-struct.cpp
index ff5521fcf3f604..17c9d6bf6a7260 100644
--- a/clang/test/CodeGen/tbaa-struct.cpp
+++ b/clang/test/CodeGen/tbaa-struct.cpp
@@ -130,7 +130,7 @@ void copy8(NamedBitfields *a1, NamedBitfields *a2) {
// CHECK-OLD: [[TS3]] = !{i64 0, i64 8, !{{.*}}, i64 0, i64 2, !{{.*}}, i64 4,
i64 8, !{{.*}}}
// CHECK-OLD: [[TS4]] = !{i64 0, i64 1, [[TAG_CHAR]], i64 1, i64 1,
[[TAG_CHAR]], i64 2, i64 1, [[TAG_CHAR]]}
// CHECK-OLD: [[TS5]] = !{i64 0, i64 1, [[TAG_CHAR]], i64 4, i64 1,
[[TAG_CHAR]], i64 5, i64 1, [[TAG_CHAR]]}
-// CHECK-OLD: [[TS6]] = !{i64 0, i64 4, [[TAG_INT]], i64 1, i64 4,
[[TAG_INT]], i64 2, i64 1, [[TAG_CHAR]], i64 8, i64 8, [[TAG_DOUBLE:!.+]]}
+// CHECK-OLD: [[TS6]] = !{i64 2, i64 1, [[TAG_CHAR]], i64 8, i64 8,
[[TAG_DOUBLE:!.+]]}
// CHECK-OLD: [[TAG_DOUBLE]] = !{[[DOUBLE:!.+]], [[DOUBLE]], i64 0}
// CHECK-OLD [[DOUBLE]] = !{!"double", [[CHAR]], i64 0}
>From 32be3b7d944fc5da50add4b6fea551ba6c9d428c Mon Sep 17 00:00:00 2001
From: Florian Hahn
Date: Mon, 26 Feb 2024 10:19:57 +
Subject: [PATCH 2/7] WIP use CGBitFieldInfo.
---
clang/lib/CodeGen/CodeGenModule.cpp | 2 +-
clang/lib/CodeGen/CodeGenTBAA.cpp | 33 +++--
clang/lib/CodeGen/CodeGenTBAA.h | 4 +++-
3 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp
b/clang/lib/CodeGen/CodeGenModule.cpp
index 95e457bef28ed3..ed8db055723024 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -397,7 +397,7 @@ CodeGenModule::CodeGenModule(ASTContext &C,
// Enable TBAA unless it's suppressed. ThreadSanitizer needs TBAA even at O0.
if (LangOpts.Sanitize.has(SanitizerKind::Thread) ||
(!CodeGenOpts.RelaxedAliasing && CodeGenOpts.OptimizationLevel > 0))
-TBAA.reset(new CodeGenTBAA(Context, TheModule, CodeGenOpts, getLangOpts(),
+TBAA.reset(new CodeGenTBAA(Context, getTypes(), TheModule, CodeGenOpts,
getLangOpts(),
getCXXABI().getMangleContext()));
// If debug info or coverage generation is enabled, create the CGDebugInfo
diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp
b/clang/lib/CodeGen/CodeGenTBAA.cpp
index 43a1aee3d73823..6a453bb1144582 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -14,6 +14,8 @@
//
//===--===//
+#include "CGRecordLayout.h"
+#include "CodeGenTypes.h"
#include "CodeGenTBAA.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Attr.h"
@@ -29,10 +31,10 @@
using namespace clang;
using namespace CodeGen;
-CodeGenTBAA::CodeGenTBAA(ASTContext &Ctx, llvm::Module &M,
+CodeGenTBAA::CodeGenTBAA(ASTContext &Ctx, CodeGenTypes &CGTypes, llvm::Module
&M,
const CodeGenOptions &CGO,
const LangOptions &Features, MangleContext &MContext)
- : Contex
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
https://github.com/efriedma-quic commented: Whitespace is weird in a few places; otherwise looks fine. https://github.com/llvm/llvm-project/pull/82922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
@@ -294,18 +297,49 @@ CodeGenTBAA::CollectFields(uint64_t BaseOffset,
return false;
const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
+const CGRecordLayout &CGRL = CGTypes.getCGRecordLayout(RD);
unsigned idx = 0;
-for (RecordDecl::field_iterator i = RD->field_begin(),
- e = RD->field_end(); i != e; ++i, ++idx) {
- if ((*i)->isZeroSize(Context) || (*i)->isUnnamedBitfield())
+for (RecordDecl::field_iterator i = RD->field_begin(), e = RD->field_end();
+ i != e;) {
+ if ((*i)->isZeroSize(Context)) {
+++i;
+++idx;
continue;
- uint64_t Offset = BaseOffset +
-Layout.getFieldOffset(idx) / Context.getCharWidth();
+ }
+
+ uint64_t Offset =
+ BaseOffset + Layout.getFieldOffset(idx) / Context.getCharWidth();
QualType FieldQTy = i->getType();
+ // Create a single field for consecutive named bitfields using char as
+ // base type.
+ if ((*i)->isBitField() && !(*i)->isUnnamedBitfield()) {
+unsigned CurrentBitFieldSize = 0;
+unsigned CurrentBitFieldOffset = CGRL.getBitFieldInfo(*i).Offset;
+while (i != e && (*i)->isBitField() && !(*i)->isUnnamedBitfield()) {
+ const CGBitFieldInfo &Info = CGRL.getBitFieldInfo(*i);
+ if (CurrentBitFieldSize + CurrentBitFieldOffset != Info.Offset)
+break;
+ CurrentBitFieldSize += Info.Size;
fhahn wrote:
Thanks, updated the patch to add a field for the first bitfield in the run
using its StorageSize, and skip bitfields where the offset in the run isn't 0
(`Offset`).
https://github.com/llvm/llvm-project/pull/82922
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
@@ -294,18 +297,49 @@ CodeGenTBAA::CollectFields(uint64_t BaseOffset,
return false;
const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
+const CGRecordLayout &CGRL = CGTypes.getCGRecordLayout(RD);
unsigned idx = 0;
-for (RecordDecl::field_iterator i = RD->field_begin(),
- e = RD->field_end(); i != e; ++i, ++idx) {
- if ((*i)->isZeroSize(Context) || (*i)->isUnnamedBitfield())
+for (RecordDecl::field_iterator i = RD->field_begin(), e = RD->field_end();
+ i != e;) {
+ if ((*i)->isZeroSize(Context)) {
+++i;
+++idx;
continue;
- uint64_t Offset = BaseOffset +
-Layout.getFieldOffset(idx) / Context.getCharWidth();
+ }
+
+ uint64_t Offset =
+ BaseOffset + Layout.getFieldOffset(idx) / Context.getCharWidth();
QualType FieldQTy = i->getType();
+ // Create a single field for consecutive named bitfields using char as
+ // base type.
+ if ((*i)->isBitField() && !(*i)->isUnnamedBitfield()) {
+unsigned CurrentBitFieldSize = 0;
+unsigned CurrentBitFieldOffset = CGRL.getBitFieldInfo(*i).Offset;
+while (i != e && (*i)->isBitField() && !(*i)->isUnnamedBitfield()) {
fhahn wrote:
Yes, test showing that added in 54cff50791dec977feb0badb74919d97dff5b859,
thanks!
https://github.com/llvm/llvm-project/pull/82922
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
https://github.com/fhahn updated https://github.com/llvm/llvm-project/pull/82922
>From 556fcefed9645aa0a0a965ff8f22d8accdf9eefc Mon Sep 17 00:00:00 2001
From: Florian Hahn
Date: Sun, 25 Feb 2024 13:23:17 +
Subject: [PATCH 1/6] [TBAA] Skip all bitfields when generating !tbaa.struct
metadata.
At the moment, clang generates what I believe are incorrect !tbaa.struct
fields for named bitfields. At the moment, the base type size is used
for named bifields (e.g. sizeof(int)) instead of the bifield width per
field. This results in overalpping fields in !tbaa.struct metadata.
This causes incorrect results when extracting individual copied fields
from !tbaa.struct as in added in dc85719d5.
This patch fixes that by skipping all bitfields, not only unnamed ones
(note that CollectFields has a TODO to support bitfields). As bitfields
specify their widths in bits, while !tbaa metadata uses bytes for sizes
and offsets, I don't think we would be able to generate correct metadata
for them in general.
If this understanding is correct, I can also extend the verifier to
check that !tbaa.struct fields aren't overlapping.
Fixes https://github.com/llvm/llvm-project/issues/82586
---
clang/lib/CodeGen/CodeGenTBAA.cpp | 2 +-
clang/test/CodeGen/tbaa-struct.cpp | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp
b/clang/lib/CodeGen/CodeGenTBAA.cpp
index dc288bc3f6157a..43a1aee3d73823 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -298,7 +298,7 @@ CodeGenTBAA::CollectFields(uint64_t BaseOffset,
unsigned idx = 0;
for (RecordDecl::field_iterator i = RD->field_begin(),
e = RD->field_end(); i != e; ++i, ++idx) {
- if ((*i)->isZeroSize(Context) || (*i)->isUnnamedBitfield())
+ if ((*i)->isZeroSize(Context) || (*i)->isBitField())
continue;
uint64_t Offset = BaseOffset +
Layout.getFieldOffset(idx) / Context.getCharWidth();
diff --git a/clang/test/CodeGen/tbaa-struct.cpp
b/clang/test/CodeGen/tbaa-struct.cpp
index ff5521fcf3f604..17c9d6bf6a7260 100644
--- a/clang/test/CodeGen/tbaa-struct.cpp
+++ b/clang/test/CodeGen/tbaa-struct.cpp
@@ -130,7 +130,7 @@ void copy8(NamedBitfields *a1, NamedBitfields *a2) {
// CHECK-OLD: [[TS3]] = !{i64 0, i64 8, !{{.*}}, i64 0, i64 2, !{{.*}}, i64 4,
i64 8, !{{.*}}}
// CHECK-OLD: [[TS4]] = !{i64 0, i64 1, [[TAG_CHAR]], i64 1, i64 1,
[[TAG_CHAR]], i64 2, i64 1, [[TAG_CHAR]]}
// CHECK-OLD: [[TS5]] = !{i64 0, i64 1, [[TAG_CHAR]], i64 4, i64 1,
[[TAG_CHAR]], i64 5, i64 1, [[TAG_CHAR]]}
-// CHECK-OLD: [[TS6]] = !{i64 0, i64 4, [[TAG_INT]], i64 1, i64 4,
[[TAG_INT]], i64 2, i64 1, [[TAG_CHAR]], i64 8, i64 8, [[TAG_DOUBLE:!.+]]}
+// CHECK-OLD: [[TS6]] = !{i64 2, i64 1, [[TAG_CHAR]], i64 8, i64 8,
[[TAG_DOUBLE:!.+]]}
// CHECK-OLD: [[TAG_DOUBLE]] = !{[[DOUBLE:!.+]], [[DOUBLE]], i64 0}
// CHECK-OLD [[DOUBLE]] = !{!"double", [[CHAR]], i64 0}
>From 32be3b7d944fc5da50add4b6fea551ba6c9d428c Mon Sep 17 00:00:00 2001
From: Florian Hahn
Date: Mon, 26 Feb 2024 10:19:57 +
Subject: [PATCH 2/6] WIP use CGBitFieldInfo.
---
clang/lib/CodeGen/CodeGenModule.cpp | 2 +-
clang/lib/CodeGen/CodeGenTBAA.cpp | 33 +++--
clang/lib/CodeGen/CodeGenTBAA.h | 4 +++-
3 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp
b/clang/lib/CodeGen/CodeGenModule.cpp
index 95e457bef28ed3..ed8db055723024 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -397,7 +397,7 @@ CodeGenModule::CodeGenModule(ASTContext &C,
// Enable TBAA unless it's suppressed. ThreadSanitizer needs TBAA even at O0.
if (LangOpts.Sanitize.has(SanitizerKind::Thread) ||
(!CodeGenOpts.RelaxedAliasing && CodeGenOpts.OptimizationLevel > 0))
-TBAA.reset(new CodeGenTBAA(Context, TheModule, CodeGenOpts, getLangOpts(),
+TBAA.reset(new CodeGenTBAA(Context, getTypes(), TheModule, CodeGenOpts,
getLangOpts(),
getCXXABI().getMangleContext()));
// If debug info or coverage generation is enabled, create the CGDebugInfo
diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp
b/clang/lib/CodeGen/CodeGenTBAA.cpp
index 43a1aee3d73823..6a453bb1144582 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -14,6 +14,8 @@
//
//===--===//
+#include "CGRecordLayout.h"
+#include "CodeGenTypes.h"
#include "CodeGenTBAA.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Attr.h"
@@ -29,10 +31,10 @@
using namespace clang;
using namespace CodeGen;
-CodeGenTBAA::CodeGenTBAA(ASTContext &Ctx, llvm::Module &M,
+CodeGenTBAA::CodeGenTBAA(ASTContext &Ctx, CodeGenTypes &CGTypes, llvm::Module
&M,
const CodeGenOptions &CGO,
const LangOptions &Features, MangleContext &MContext)
- : Contex
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
@@ -294,18 +297,49 @@ CodeGenTBAA::CollectFields(uint64_t BaseOffset,
return false;
const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
+const CGRecordLayout &CGRL = CGTypes.getCGRecordLayout(RD);
unsigned idx = 0;
-for (RecordDecl::field_iterator i = RD->field_begin(),
- e = RD->field_end(); i != e; ++i, ++idx) {
- if ((*i)->isZeroSize(Context) || (*i)->isUnnamedBitfield())
+for (RecordDecl::field_iterator i = RD->field_begin(), e = RD->field_end();
+ i != e;) {
+ if ((*i)->isZeroSize(Context)) {
+++i;
+++idx;
continue;
- uint64_t Offset = BaseOffset +
-Layout.getFieldOffset(idx) / Context.getCharWidth();
+ }
+
+ uint64_t Offset =
+ BaseOffset + Layout.getFieldOffset(idx) / Context.getCharWidth();
QualType FieldQTy = i->getType();
+ // Create a single field for consecutive named bitfields using char as
+ // base type.
+ if ((*i)->isBitField() && !(*i)->isUnnamedBitfield()) {
+unsigned CurrentBitFieldSize = 0;
+unsigned CurrentBitFieldOffset = CGRL.getBitFieldInfo(*i).Offset;
+while (i != e && (*i)->isBitField() && !(*i)->isUnnamedBitfield()) {
efriedma-quic wrote:
I don't think this handles non-zero-length unnamed bitfields correctly; they
count as part of a series of bitfields.
https://github.com/llvm/llvm-project/pull/82922
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
@@ -294,18 +297,49 @@ CodeGenTBAA::CollectFields(uint64_t BaseOffset,
return false;
const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
+const CGRecordLayout &CGRL = CGTypes.getCGRecordLayout(RD);
unsigned idx = 0;
-for (RecordDecl::field_iterator i = RD->field_begin(),
- e = RD->field_end(); i != e; ++i, ++idx) {
- if ((*i)->isZeroSize(Context) || (*i)->isUnnamedBitfield())
+for (RecordDecl::field_iterator i = RD->field_begin(), e = RD->field_end();
+ i != e;) {
+ if ((*i)->isZeroSize(Context)) {
+++i;
+++idx;
continue;
- uint64_t Offset = BaseOffset +
-Layout.getFieldOffset(idx) / Context.getCharWidth();
+ }
+
+ uint64_t Offset =
+ BaseOffset + Layout.getFieldOffset(idx) / Context.getCharWidth();
QualType FieldQTy = i->getType();
+ // Create a single field for consecutive named bitfields using char as
+ // base type.
+ if ((*i)->isBitField() && !(*i)->isUnnamedBitfield()) {
+unsigned CurrentBitFieldSize = 0;
+unsigned CurrentBitFieldOffset = CGRL.getBitFieldInfo(*i).Offset;
+while (i != e && (*i)->isBitField() && !(*i)->isUnnamedBitfield()) {
+ const CGBitFieldInfo &Info = CGRL.getBitFieldInfo(*i);
+ if (CurrentBitFieldSize + CurrentBitFieldOffset != Info.Offset)
+break;
+ CurrentBitFieldSize += Info.Size;
efriedma-quic wrote:
Can we pull the initial offset and total length of the whole run bitfields out
of the CGBitFieldInfo, instead of trying to add up the sizes of the individual
fields? I think it should be StorageOffset/StorageSize.
https://github.com/llvm/llvm-project/pull/82922
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
fhahn wrote: > This seems like it messes up the metadata in a different way: we have no > representation of the bitfield at all, so it looks like it's padding. > > I think we want to treat multiple adjacent bitfields as a single "field". So > NamedBitfields from the testcase would have three fields in the LLVM TBAA > data: one "field" containing both bitfields, followed by a field for the > char, followed by a field for the double. You should be able to use > CGBitFieldInfo for this, I think? Thanks, updated the patch as suggested. https://github.com/llvm/llvm-project/pull/82922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
https://github.com/fhahn edited https://github.com/llvm/llvm-project/pull/82922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [TBAA] Handle bitfields when generating !tbaa.struct metadata. (PR #82922)
https://github.com/fhahn edited https://github.com/llvm/llvm-project/pull/82922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
