[Clang] Add __datasizeof #67805
[Clang] Add __datasizeof #67805
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang ChangesThe data size is required for implementing the Full diff: https://github.com/llvm/llvm-project/pull/67805.diff 8 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index d2656310e79c9b8..cd9d84d8e59992a 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -1016,4 +1016,7 @@ def warn_unpacked_field
def warn_unaligned_access : Warning<
"field %1 within %0 is less aligned than %2 and is usually due to %0 being "
"packed, which can lead to unaligned accesses">, InGroup<UnalignedAccess>, DefaultIgnore;
+
+def err_cannot_mangle_expression : Error<
+ "cannot yet mangle %0 expression">;
}
diff --git a/clang/include/clang/Basic/Features.def b/clang/include/clang/Basic/Features.def
index cbeb92fbe4fdd19..d8ca9395368cbbc 100644
--- a/clang/include/clang/Basic/Features.def
+++ b/clang/include/clang/Basic/Features.def
@@ -236,6 +236,7 @@ FEATURE(shadow_call_stack,
FEATURE(tls, PP.getTargetInfo().isTLSSupported())
FEATURE(underlying_type, LangOpts.CPlusPlus)
FEATURE(experimental_library, LangOpts.ExperimentalLibrary)
+FEATURE(datasizeof, LangOpts.CPlusPlus)
// C11 features supported by other languages as extensions.
EXTENSION(c_alignas, true)
diff --git a/clang/include/clang/Basic/TokenKinds.def b/clang/include/clang/Basic/TokenKinds.def
index 72e8df8c793a7b6..0870613eb5a6cad 100644
--- a/clang/include/clang/Basic/TokenKinds.def
+++ b/clang/include/clang/Basic/TokenKinds.def
@@ -310,6 +310,7 @@ KEYWORD(return , KEYALL)
KEYWORD(short , KEYALL)
KEYWORD(signed , KEYALL)
UNARY_EXPR_OR_TYPE_TRAIT(sizeof, SizeOf, KEYALL)
+UNARY_EXPR_OR_TYPE_TRAIT(__datasizeof, DataSizeOf, KEYCXX)
KEYWORD(static , KEYALL)
KEYWORD(struct , KEYALL)
KEYWORD(switch , KEYALL)
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index fea06b97259fe31..492e22db7826a00 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -3226,9 +3226,14 @@ static bool HandleLValueIndirectMember(EvalInfo &Info, const Expr *E,
return true;
}
+enum class SizeOfType {
+ SizeOf,
+ DataSizeOf,
+};
+
/// Get the size of the given type in char units.
-static bool HandleSizeof(EvalInfo &Info, SourceLocation Loc,
- QualType Type, CharUnits &Size) {
+static bool HandleSizeof(EvalInfo &Info, SourceLocation Loc, QualType Type,
+ CharUnits &Size, SizeOfType SOT = SizeOfType::SizeOf) {
// sizeof(void), __alignof__(void), sizeof(function) = 1 as a gcc
// extension.
if (Type->isVoidType() || Type->isFunctionType()) {
@@ -3248,7 +3253,10 @@ static bool HandleSizeof(EvalInfo &Info, SourceLocation Loc,
return false;
}
- Size = Info.Ctx.getTypeSizeInChars(Type);
+ if (SOT == SizeOfType::SizeOf)
+ Size = Info.Ctx.getTypeSizeInChars(Type);
+ else
+ Size = Info.Ctx.getTypeInfoDataSizeInChars(Type).Width;
return true;
}
@@ -13548,6 +13556,7 @@ bool IntExprEvaluator::VisitUnaryExprOrTypeTraitExpr(
return Success(1, E);
}
+ case UETT_DataSizeOf:
case UETT_SizeOf: {
QualType SrcTy = E->getTypeOfArgument();
// C++ [expr.sizeof]p2: "When applied to a reference or a reference type,
@@ -13556,8 +13565,11 @@ bool IntExprEvaluator::VisitUnaryExprOrTypeTraitExpr(
SrcTy = Ref->getPointeeType();
CharUnits Sizeof;
- if (!HandleSizeof(Info, E->getExprLoc(), SrcTy, Sizeof))
+ if (!HandleSizeof(Info, E->getExprLoc(), SrcTy, Sizeof,
+ E->getKind() == UETT_DataSizeOf ? SizeOfType::DataSizeOf
+ : SizeOfType::SizeOf)) {
return false;
+ }
return Success(Sizeof, E);
}
case UETT_OpenMPRequiredSimdAlign:
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index e7a5a6b6b8119c0..92c2dfead7f6c25 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -28,6 +28,7 @@
#include "clang/AST/Mangle.h"
#include "clang/AST/TypeLoc.h"
#include "clang/Basic/ABI.h"
+#include "clang/Basic/DiagnosticAST.h"
#include "clang/Basic/Module.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/TargetInfo.h"
@@ -5038,19 +5039,19 @@ void CXXNameMangler::mangleExpression(const Expr *E, unsigned Arity,
Out << 'a';
MangleAlignofSizeofArg();
break;
+ case UETT_DataSizeOf: {
+ Context.getDiags().Report(diag::err_cannot_mangle_expression)
+ << "__datasizeof";
+ return;
+ }
case UETT_VecStep: {
- DiagnosticsEngine &Diags = Context.getDiags();
- unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
- "cannot yet mangle vec_step expression");
- Diags.Report(DiagID);
+ Context.getDiags().Report(diag::err_cannot_mangle_expression)
+ << "vec_step";
return;
}
case UETT_OpenMPRequiredSimdAlign: {
- DiagnosticsEngine &Diags = Context.getDiags();
- unsigned DiagID = Diags.getCustomDiagID(
- DiagnosticsEngine::Error,
- "cannot yet mangle __builtin_omp_required_simd_align expression");
- Diags.Report(DiagID);
+ Context.getDiags().Report(diag::err_cannot_mangle_expression)
+ << "__builtin_omp_required_simd_align";
return;
}
}
diff --git a/clang/lib/Parse/ParseExpr.cpp b/clang/lib/Parse/ParseExpr.cpp
index 74664c34abdbd89..8d8be8960ea2736 100644
--- a/clang/lib/Parse/ParseExpr.cpp
+++ b/clang/lib/Parse/ParseExpr.cpp
@@ -1451,6 +1451,9 @@ ExprResult Parser::ParseCastExpression(CastParseKind ParseKind,
// unary-expression: '__alignof' '(' type-name ')'
case tok::kw_sizeof: // unary-expression: 'sizeof' unary-expression
// unary-expression: 'sizeof' '(' type-name ')'
+ // unary-expression: '__datasizeof' unary-expression
+ // unary-expression: '__datasizeof' '(' type-name ')'
+ case tok::kw___datasizeof:
case tok::kw_vec_step: // unary-expression: OpenCL 'vec_step' expression
// unary-expression: '__builtin_omp_required_simd_align' '(' type-name ')'
case tok::kw___builtin_omp_required_simd_align:
@@ -2297,6 +2300,8 @@ Parser::ParsePostfixExpressionSuffix(ExprResult LHS) {
/// unary-expression: [C99 6.5.3]
/// 'sizeof' unary-expression
/// 'sizeof' '(' type-name ')'
+/// [Clang] '__datasizeof' unary-expression
+/// [Clang] '__datasizeof' '(' type-name ')'
/// [GNU] '__alignof' unary-expression
/// [GNU] '__alignof' '(' type-name ')'
/// [C11] '_Alignof' '(' type-name ')'
@@ -2325,8 +2330,8 @@ Parser::ParseExprAfterUnaryExprOrTypeTrait(const Token &OpTok,
SourceRange &CastRange) {
assert(OpTok.isOneOf(tok::kw_typeof, tok::kw_typeof_unqual, tok::kw_sizeof,
- tok::kw___alignof, tok::kw_alignof, tok::kw__Alignof,
- tok::kw_vec_step,
+ tok::kw___datasizeof, tok::kw___alignof, tok::kw_alignof,
+ tok::kw__Alignof, tok::kw_vec_step,
tok::kw___builtin_omp_required_simd_align) &&
"Not a typeof/sizeof/alignof/vec_step expression!");
@@ -2440,14 +2445,16 @@ ExprResult Parser::ParseSYCLUniqueStableNameExpression() {
/// 'sizeof' unary-expression
/// 'sizeof' '(' type-name ')'
/// [C++11] 'sizeof' '...' '(' identifier ')'
+/// [Clang] '__datasizeof' unary-expression
+/// [Clang] '__datasizeof' '(' type-name ')'
/// [GNU] '__alignof' unary-expression
/// [GNU] '__alignof' '(' type-name ')'
/// [C11] '_Alignof' '(' type-name ')'
/// [C++11] 'alignof' '(' type-id ')'
/// \endverbatim
ExprResult Parser::ParseUnaryExprOrTypeTraitExpression() {
- assert(Tok.isOneOf(tok::kw_sizeof, tok::kw___alignof, tok::kw_alignof,
- tok::kw__Alignof, tok::kw_vec_step,
+ assert(Tok.isOneOf(tok::kw_sizeof, tok::kw___datasizeof, tok::kw___alignof,
+ tok::kw_alignof, tok::kw__Alignof, tok::kw_vec_step,
tok::kw___builtin_omp_required_simd_align) &&
"Not a sizeof/alignof/vec_step expression!");
Token OpTok = Tok;
@@ -2519,14 +2526,26 @@ ExprResult Parser::ParseUnaryExprOrTypeTraitExpression() {
CastRange);
UnaryExprOrTypeTrait ExprKind = UETT_SizeOf;
- if (OpTok.isOneOf(tok::kw_alignof, tok::kw__Alignof))
+ switch (OpTok.getKind()) {
+ case tok::kw_alignof:
+ case tok::kw__Alignof:
ExprKind = UETT_AlignOf;
- else if (OpTok.is(tok::kw___alignof))
+ break;
+ case tok::kw___alignof:
ExprKind = UETT_PreferredAlignOf;
- else if (OpTok.is(tok::kw_vec_step))
+ break;
+ case tok::kw_vec_step:
ExprKind = UETT_VecStep;
- else if (OpTok.is(tok::kw___builtin_omp_required_simd_align))
+ break;
+ case tok::kw___builtin_omp_required_simd_align:
ExprKind = UETT_OpenMPRequiredSimdAlign;
+ break;
+ case tok::kw___datasizeof:
+ ExprKind = UETT_DataSizeOf;
+ break;
+ default:
+ break;
+ }
if (isCastExpr)
return Actions.ActOnUnaryExprOrTypeTraitExpr(OpTok.getLocation(),
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 92496b03ecabe54..b8daee02951a520 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -4428,8 +4428,9 @@ bool Sema::CheckUnaryExprOrTypeTraitOperand(Expr *E,
assert(!ExprTy->isReferenceType());
bool IsUnevaluatedOperand =
- (ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf ||
- ExprKind == UETT_PreferredAlignOf || ExprKind == UETT_VecStep);
+ (ExprKind == UETT_SizeOf || ExprKind == UETT_DataSizeOf ||
+ ExprKind == UETT_AlignOf || ExprKind == UETT_PreferredAlignOf ||
+ ExprKind == UETT_VecStep);
if (IsUnevaluatedOperand) {
ExprResult Result = CheckUnevaluatedOperand(E);
if (Result.isInvalid())
diff --git a/clang/test/SemaCXX/datasizeof.cpp b/clang/test/SemaCXX/datasizeof.cpp
new file mode 100644
index 000000000000000..5f7169bd86f0b99
--- /dev/null
+++ b/clang/test/SemaCXX/datasizeof.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-linux-gnu -verify %s
+
+// expected-no-diagnostics
+
+#if !__has_feature(datasizeof)
+# error "Expected datasizeof feature"
+#endif
+
+struct HasPadding {
+ int i;
+ char c;
+};
+
+struct HasUsablePadding {
+ int i;
+ char c;
+
+ HasUsablePadding() {}
+};
+
+static_assert(__datasizeof(int) == 4);
+static_assert(__datasizeof(HasPadding) == 8);
+static_assert(__datasizeof(HasUsablePadding) == 5);
+
+static_assert([] {
+ int* p = nullptr;
+ HasPadding* p2 = nullptr;
+ HasUsablePadding* p3 = nullptr;
+ static_assert(__datasizeof(*p) == 4);
+ static_assert(__datasizeof(*p2) == 8);
+ static_assert(__datasizeof(*p3) == 5);
+
+ return true;
+}());
|
clang/lib/AST/ItaniumMangle.cpp
Outdated
| DiagnosticsEngine::Error, | ||
| "cannot yet mangle __builtin_omp_required_simd_align expression"); | ||
| Diags.Report(DiagID); | ||
| Context.getDiags().Report(diag::err_cannot_mangle_expression) |
There was a problem hiding this comment.
It looks like we have one other case we can replace as well:
llvm-project/clang/lib/AST/ItaniumMangle.cpp
Line 4728 in 9782993
There was a problem hiding this comment.
I think we should keep the more verbose form as it's more of a clear signal to the reader "this is weird and needs to be fixed". (These diagnostics are also strange in that they're not associated with a source location, which looks like a bug but isn't really one because we expect these diagnostics to go away at some point.)
The data size is required for implementing the memmove optimization for std::copy, std::move etc. correctly as well as replacing __compressed_pair with [[no_unique_address]] in libc++. Since the compiler already knows the data size, we can avoid some complexity by exposing that information. Patch by Nikolas Klauser! Differential Review: llvm#67805
|
|
||
| def err_cannot_mangle_expression : Error< | ||
| "cannot yet mangle %0 expression">; |
There was a problem hiding this comment.
All of the other "cannot yet mangle" diagnostics are done with a custom diagnostic, as in
llvm-project/clang/lib/AST/ItaniumMangle.cpp
Line 5116 in 475e154
There was a problem hiding this comment.
I think a // FIXME would make that a lot more obvious than using the custom diagnostics engine. To me it wasn't obvious that these should be FIXMES, which is the reason I replaced them with a normal diagnostic.
There was a problem hiding this comment.
Ah, so the "obvious" may be more obvious to people used to working with Clang's diagnostics and not so obvious to folks not as well-versed. Good feedback!
I'm okay with the changes but I have a preference for doing this in two steps. Step 1: your patch w/custom diagnostics engine, Step 2: replace all custom diagnostics with the regular diagnostic in one go. (This makes code archeology easier because the changes are kind of orthogonal to one another.) Would you be okay with splitting it up despite the extra work?
| @@ -236,6 +236,7 @@ FEATURE(shadow_call_stack, | |||
| FEATURE(tls, PP.getTargetInfo().isTLSSupported()) | |||
| FEATURE(underlying_type, LangOpts.CPlusPlus) | |||
| FEATURE(experimental_library, LangOpts.ExperimentalLibrary) | |||
| FEATURE(datasizeof, LangOpts.CPlusPlus) | |||
There was a problem hiding this comment.
FEATURE is for standards features, this should be exposed under EXTENSION instead.
There was a problem hiding this comment.
The problem with EXTENSION is that it's influenced by -pedantic-errors, which makes it pretty much useless for standard libraries.
There was a problem hiding this comment.
Yeahhhhh, but we still need to diagnose it as an extension because that's what it is. (We should not add extensions that we do not diagnose under -pedantic -- we have been inconsistent in the past but we've gotten better in the past several years.)
Pedantic warnings aren't triggered in system headers, so I think I'm failing to see the concern: https://godbolt.org/z/nn7TdKrcv
There was a problem hiding this comment.
The problem is that -pedantic-errors makes __has_extension useless: https://godbolt.org/z/7TfMjf5h6
There was a problem hiding this comment.
Oh my. That behavior makes sense, but is really not helpful in this case.
You could use compiler version macro checks instead of __has_extension. That's pretty gross and likely easy to forget, but would it be a viable workaround?
There was a problem hiding this comment.
Yeah, that's what we'd have to fall back to - or just live with using the fallback when changing warning flags (!?).
Not really relevant for this patch, but I'm not sure the behaviour actually makes that much sense. There are a few things that make it somewhat weird IMO.
-pedantic-errorsdisables__has_extension, but-Werror=pedanticdoesn't. These are clearly distinct flags, but for a user they look like aliases, since they interact with warnings identically (AFAIK).- If you use
__has_extensionyou almost always want to know whether the compiler accepts some code, and if not you generate an error that you don't support the compiler or use some fallback implementation. (At least that's my interpretation) - Changing diagnostic flags shouldn't influence anything other than the behaviour of the diagnostics engine. The code shouldn't compile differently, you should just get different diagnostics.
clang/lib/AST/ItaniumMangle.cpp
Outdated
| DiagnosticsEngine::Error, | ||
| "cannot yet mangle __builtin_omp_required_simd_align expression"); | ||
| Diags.Report(DiagID); | ||
| Context.getDiags().Report(diag::err_cannot_mangle_expression) |
There was a problem hiding this comment.
I think we should keep the more verbose form as it's more of a clear signal to the reader "this is weird and needs to be fixed". (These diagnostics are also strange in that they're not associated with a source location, which looks like a bug but isn't really one because we expect these diagnostics to go away at some point.)
philnik777
force-pushed
the
datasizeof
branch
from
547d1fb to
e6770ca
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
philnik777
force-pushed
the
datasizeof
branch
from
e6770ca to
f384bc5
Compare
|
@AaronBallman This did indeed catch a bug. Also, I think there was something about how codegen tests should be written, but I don't remember exactly anymore. Could you take a look at the test? |
clang/test/CodeGenCXX/datasizeof.cpp
Outdated
| // RUN: %clang_cc1 -triple x86_64 -emit-llvm %s -o - | FileCheck %s | ||
|
|
||
| // CHECK: define dso_local noundef i32 @_Z4testi(i32 noundef %i) #0 { | ||
| // CHECK-NEXT: entry: | ||
| // CHECK-NEXT: %i.addr = alloca i32, align 4 | ||
| // CHECK-NEXT: store i32 %i, ptr %i.addr, align 4 | ||
| // CHECK-NEXT: %0 = load i32, ptr %i.addr, align 4 | ||
| // CHECK-NEXT: %inc = add nsw i32 %0, 1 | ||
| // CHECK-NEXT: store i32 %inc, ptr %i.addr, align 4 | ||
| // CHECK-NEXT: %1 = zext i32 %0 to i64 | ||
| // CHECK-NEXT: %2 = mul nuw i64 4, %1 | ||
| // CHECK-NEXT: %3 = load i32, ptr %i.addr, align 4 | ||
| // CHECK-NEXT: ret i32 %3 | ||
| // CHECK-NEXT: } |
There was a problem hiding this comment.
I think you may need to add a more specific triple because of mangling differences between MSVC and Itanium. You should also replace the %<stuff> form to use a regex.
Actually, it might be worth generating the test comments via llvm/utils/update_cc_test_checks.py
philnik777
force-pushed
the
datasizeof
branch
from
f384bc5 to
8010f61
Compare
philnik777
force-pushed
the
datasizeof
branch
from
8010f61 to
7174224
Compare
The data size is required for implementing the `memmove` optimization for `std::copy`, `std::move` etc. correctly as well as replacing `__compressed_pair` with `[[no_unique_address]]` in libc++. Since the compiler already knows the data size, we can avoid some complexity by exposing that information.
The data size is required for implementing the
memmoveoptimization forstd::copy,std::moveetc. correctly as well as replacing__compressed_pairwith[[no_unique_address]]in libc++. Since the compiler already knows the data size, we can avoid some complexity by exposing that information.