-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang] Implement -fstrict-bool #160790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[clang] Implement -fstrict-bool #160790
Conversation
``bool`` values are stored as i8 in memory, and it is undefined
behavior for a ``bool`` value to be any value other than 0 or 1. Clang
exploits this with range metadata: ``bool`` load instructions at any
optimization level above -O0 are assumed to only have their lowest bit
set. This can create memory safety problems when other bits are set,
for instance through ``memcpy``.
This change allows users to configure this behavior in three ways:
* ``-fstrict-bool`` represents the status quo; range metadata is added at levels
above -O0 and allows the compiler to assume in-memory ``bool`` values are
always either 0 or 1.
* ``-fno-strict-bool[={truncate|nonzero}]`` disables range metadata on ``bool``
loaded values and offers two ways to interpret the loaded values. ``truncate``
means the value is true is the least significant bit is 1 and false otherwise;
``nonzero`` means the value is true if any bit is 1 and false otherwise.
The default is ``-fstrict-bool`` to not change the current behavior of Clang.
The default behavior of ``-fno-strict-bool`` is ``truncate``.
Radar-ID: 139397212
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: None (apple-fcloutier) ChangesThis PR implements the -fstrict-bool RFC.
This change allows users to configure this behavior in three ways:
The default is Radar-ID: 139397212 Full diff: https://github.com/llvm/llvm-project/pull/160790.diff 9 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9744c4f17610b..85453a8fff75a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -250,6 +250,7 @@ New Compiler Flags
------------------
- New option ``-fno-sanitize-debug-trap-reasons`` added to disable emitting trap reasons into the debug info when compiling with trapping UBSan (e.g. ``-fsanitize-trap=undefined``).
- New option ``-fsanitize-debug-trap-reasons=`` added to control emitting trap reasons into the debug info when compiling with trapping UBSan (e.g. ``-fsanitize-trap=undefined``).
+- New option ``-f[no-]strict-bool`` added to control whether Clang can assume that ``bool`` values loaded from memory cannot have a bit pattern other than 0 or 1.
Lanai Support
diff --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
index a8bbf146431ea..e0719e16fc44c 100644
--- a/clang/docs/UsersManual.rst
+++ b/clang/docs/UsersManual.rst
@@ -2306,6 +2306,24 @@ are listed below.
additional function arity information (for supported targets). See
:doc:`ControlFlowIntegrity` for more details.
+.. option:: -fstrict-bool
+
+ ``bool`` values are stored to memory as 8-bit values on most targets. Under
+ ``-fstrict-bool``, it is undefined behavior for a ``bool`` value stored in
+ memory to have any other bit pattern than 0 or 1. This creates some
+ optimization opportunities for the compiler, but it enables memory
+ corruption if that assumption is violated, for instance if any other value
+ is ``memcpy``ed over a ``bool``. This is enabled by default.
+
+.. option:: -fno-strict-bool[={truncate|nonzero}]
+
+ Disable optimizations based on the assumption that all ``bool`` values,
+ which are typically represented as 8-bit integers in memory, only ever
+ contain bit patterns 0 or 1. When =truncate is specified, a ``bool`` is
+ true if its least significant bit is set, and false otherwise. When =nonzero
+ is specified, a ``bool`` is true when any bit is set, and false otherwise.
+ The default is =truncate.
+
.. option:: -fstrict-vtable-pointers
Enable optimizations based on the strict rules for overwriting polymorphic
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index 872f73ebf3810..a09e65698a54d 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -313,6 +313,7 @@ CODEGENOPT(SoftFloat , 1, 0, Benign) ///< -soft-float.
CODEGENOPT(SpeculativeLoadHardening, 1, 0, Benign) ///< Enable speculative load hardening.
CODEGENOPT(FineGrainedBitfieldAccesses, 1, 0, Benign) ///< Enable fine-grained bitfield accesses.
CODEGENOPT(StrictEnums , 1, 0, Benign) ///< Optimize based on strict enum definition.
+ENUM_CODEGENOPT(LoadBoolFromMem, BoolFromMem, 2, BoolFromMem::Strict, Benign) ///> Optimize based on in-memory bool values being 0 or 1.
CODEGENOPT(StrictVTablePointers, 1, 0, Benign) ///< Optimize based on the strict vtable pointers
CODEGENOPT(TimePasses , 1, 0, Benign) ///< Set when -ftime-report, -ftime-report=, -ftime-report-json, or -stats-file-timers is enabled.
CODEGENOPT(TimePassesPerRun , 1, 0, Benign) ///< Set when -ftime-report=per-pass-run is enabled.
diff --git a/clang/include/clang/Basic/CodeGenOptions.h b/clang/include/clang/Basic/CodeGenOptions.h
index 5d5cf250b56b9..4d9642135d714 100644
--- a/clang/include/clang/Basic/CodeGenOptions.h
+++ b/clang/include/clang/Basic/CodeGenOptions.h
@@ -208,6 +208,16 @@ class CodeGenOptions : public CodeGenOptionsBase {
///< larger debug info than `Basic`.
};
+ enum BoolFromMem {
+ Strict, ///< In-memory bool values are assumed to be 0 or 1, and any other
+ ///< value is UB.
+ Truncate, ///< Convert in-memory bools to i1 by checking if the least
+ ///< significant bit is 1.
+ NonZero, ///< Convert in-memory bools to i1 by checking if any bit is set
+ ///< to 1.
+ NonStrictDefault = Truncate
+ };
+
/// The code model to use (-mcmodel).
std::string CodeModel;
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 77f19a240a7f9..bd009c409f854 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4137,6 +4137,27 @@ def fno_debug_macro : Flag<["-"], "fno-debug-macro">, Group<f_Group>,
def fstrict_aliasing : Flag<["-"], "fstrict-aliasing">, Group<f_Group>,
Visibility<[ClangOption, CLOption, DXCOption]>,
HelpText<"Enable optimizations based on strict aliasing rules">;
+def fstrict_bool : Flag<["-"], "fstrict-bool">, Group<f_Group>,
+ Visibility<[ClangOption]>,
+ HelpText<"Enable optimizations based on bool bit patterns never being "
+ "anything other than 0 or 1">;
+def fno_strict_bool : Flag<["-"], "fno-strict-bool">, Group<f_Group>,
+ Visibility<[ClangOption]>,
+ HelpText<"Disable optimizations based on bool bit patterns never being "
+ "anything other than 0 or 1">;
+def fno_strict_bool_EQ : Joined<["-"], "fno-strict-bool=">, Group<f_Group>,
+ Visibility<[ClangOption]>,
+ HelpText<"Disable optimizations based on bool bit patterns never being "
+ "anything other than 0 or 1, specifying a conversion behavior.">,
+ Values<"truncate,nonzero">;
+def load_bool_from_mem : Joined<["-"], "load-bool-from-mem=">, Group<f_Group>,
+ Visibility<[CC1Option]>,
+ HelpText<"Specify how to convert a multi-bit bool loaded from memory to a "
+ "1-bit value">,
+ NormalizedValuesScope<"CodeGenOptions::BoolFromMem">,
+ Values<"strict,nonstrict,truncate,nonzero">,
+ NormalizedValues<["Strict", "NonStrictDefault", "Truncate", "NonZero"]>,
+ MarshallingInfoEnum<CodeGenOpts<"LoadBoolFromMem">, "Strict">;
def fstrict_enums : Flag<["-"], "fstrict-enums">, Group<f_Group>,
Visibility<[ClangOption, CC1Option]>,
HelpText<"Enable optimizations based on the strict definition of an enum's "
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index e6e4947882544..3a22af9e52eb0 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -1981,9 +1981,9 @@ llvm::Value *CodeGenFunction::EmitLoadOfScalar(LValue lvalue,
lvalue.getTBAAInfo(), lvalue.isNontemporal());
}
-static bool getRangeForType(CodeGenFunction &CGF, QualType Ty,
- llvm::APInt &Min, llvm::APInt &End,
- bool StrictEnums, bool IsBool) {
+static bool getRangeForType(CodeGenFunction &CGF, QualType Ty, llvm::APInt &Min,
+ llvm::APInt &End, bool StrictEnums, bool StrictBool,
+ bool IsBool) {
const auto *ED = Ty->getAsEnumDecl();
bool IsRegularCPlusPlusEnum =
CGF.getLangOpts().CPlusPlus && StrictEnums && ED && !ED->isFixed();
@@ -1991,6 +1991,8 @@ static bool getRangeForType(CodeGenFunction &CGF, QualType Ty,
return false;
if (IsBool) {
+ if (!StrictBool)
+ return false;
Min = llvm::APInt(CGF.getContext().getTypeSize(Ty), 0);
End = llvm::APInt(CGF.getContext().getTypeSize(Ty), 2);
} else {
@@ -2001,7 +2003,10 @@ static bool getRangeForType(CodeGenFunction &CGF, QualType Ty,
llvm::MDNode *CodeGenFunction::getRangeForLoadFromType(QualType Ty) {
llvm::APInt Min, End;
+ bool IsStrictBool =
+ CGM.getCodeGenOpts().getLoadBoolFromMem() == CodeGenOptions::Strict;
if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums,
+ IsStrictBool,
Ty->hasBooleanRepresentation() && !Ty->isVectorType()))
return nullptr;
@@ -2049,7 +2054,8 @@ bool CodeGenFunction::EmitScalarRangeCheck(llvm::Value *Value, QualType Ty,
return false;
llvm::APInt Min, End;
- if (!getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool))
+ if (!getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true,
+ /*StrictBool=*/true, IsBool))
return true;
SanitizerKind::SanitizerOrdinal Kind =
@@ -2199,8 +2205,14 @@ llvm::Value *CodeGenFunction::EmitFromMemory(llvm::Value *Value, QualType Ty) {
llvm::Type *ResTy = ConvertType(Ty);
if (Ty->hasBooleanRepresentation() || Ty->isBitIntType() ||
- Ty->isExtVectorBoolType())
- return Builder.CreateTrunc(Value, ResTy, "loadedv");
+ Ty->isExtVectorBoolType()) {
+ if (CGM.getCodeGenOpts().getLoadBoolFromMem() == CodeGenOptions::NonZero) {
+ auto *NonZero = Builder.CreateICmpNE(
+ Value, llvm::Constant::getNullValue(Value->getType()), "loadedv.nz");
+ return Builder.CreateIntCast(NonZero, ResTy, false, "loadedv");
+ } else
+ return Builder.CreateTrunc(Value, ResTy, "loadedv");
+ }
return Value;
}
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 8b1637ced5730..e2aa74b725b18 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5815,6 +5815,28 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
if (!Args.hasFlag(options::OPT_fstruct_path_tbaa,
options::OPT_fno_struct_path_tbaa, true))
CmdArgs.push_back("-no-struct-path-tbaa");
+
+ if (Arg *A = Args.getLastArg(options::OPT_fstrict_bool,
+ options::OPT_fno_strict_bool,
+ options::OPT_fno_strict_bool_EQ)) {
+ StringRef BFM = "";
+ if (A->getOption().matches(options::OPT_fstrict_bool))
+ BFM = "strict";
+ else if (A->getOption().matches(options::OPT_fno_strict_bool))
+ BFM = "nonstrict";
+ else if (A->getValue() == StringRef("truncate"))
+ BFM = "truncate";
+ else if (A->getValue() == StringRef("nonzero"))
+ BFM = "nonzero";
+ else
+ D.Diag(diag::err_drv_invalid_value)
+ << A->getAsString(Args) << A->getValue();
+ CmdArgs.push_back(Args.MakeArgString("-load-bool-from-mem=" + BFM));
+ } else if (KernelOrKext) {
+ // If unspecified, assume -fno-strict-bool=truncate in the Darwin kernel.
+ CmdArgs.push_back("-load-bool-from-mem=truncate");
+ }
+
Args.addOptInFlag(CmdArgs, options::OPT_fstrict_enums,
options::OPT_fno_strict_enums);
Args.addOptOutFlag(CmdArgs, options::OPT_fstrict_return,
diff --git a/clang/test/CodeGen/strict-bool.c b/clang/test/CodeGen/strict-bool.c
new file mode 100644
index 0000000000000..f95cade48d7aa
--- /dev/null
+++ b/clang/test/CodeGen/strict-bool.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple armv7-apple-darwin -O1 -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK-STRICT
+// RUN: %clang_cc1 -triple armv7-apple-darwin -O1 -load-bool-from-mem=strict -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK-STRICT
+// RUN: %clang_cc1 -triple armv7-apple-darwin -O1 -load-bool-from-mem=nonstrict -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK-TRUNCATE
+// RUN: %clang_cc1 -triple armv7-apple-darwin -O1 -load-bool-from-mem=truncate -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK-TRUNCATE
+// RUN: %clang_cc1 -triple armv7-apple-darwin -O1 -load-bool-from-mem=nonzero -emit-llvm -o - %s | FileCheck %s -check-prefix=CHECK-NONZERO
+
+struct has_bool {
+ _Bool b;
+};
+
+int foo(struct has_bool *b) {
+ // CHECK-STRICT: load i8, {{.*}}, !range ![[RANGE_BOOL:[0-9]+]]
+ // CHECK-STRICT-NOT: and i8
+
+ // CHECK-TRUNCATE: [[BOOL:%.+]] = load i8
+ // CHECK-TRUNCATE: and i8 [[BOOL]], 1
+
+ // CHECK-NONZERO: [[BOOL:%.+]] = load i8
+ // CHECK-NONZERO: cmp ne i8 [[BOOL]], 0
+ return b->b;
+}
+
+// CHECK_STRICT: ![[RANGE_BOOL]] = !{i8 0, i8 2}
diff --git a/clang/test/Driver/strict-bool.c b/clang/test/Driver/strict-bool.c
new file mode 100644
index 0000000000000..dc1a25872324b
--- /dev/null
+++ b/clang/test/Driver/strict-bool.c
@@ -0,0 +1,22 @@
+// RUN: %clang -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-NONE
+// RUN: %clang -### -fstrict-bool %s 2>&1 | FileCheck %s --check-prefix=CHECK-STRICT
+// RUN: %clang -### -fno-strict-bool %s 2>&1 | FileCheck %s --check-prefix=CHECK-NONSTRICT
+// RUN: %clang -### -fno-strict-bool=truncate %s 2>&1 | FileCheck %s --check-prefix=CHECK-TRUNCATE
+// RUN: %clang -### -fno-strict-bool=nonzero %s 2>&1 | FileCheck %s --check-prefix=CHECK-NONZERO
+// RUN: %clang -### -fstrict-bool -fno-strict-bool %s 2>&1 | FileCheck %s --check-prefix=CHECK-NONSTRICT
+// RUN: %clang -### -fno-strict-bool -fno-strict-bool=nonzero %s 2>&1 | FileCheck %s --check-prefix=CHECK-NONZERO
+// RUN: %clang -### -fno-strict-bool=nonzero -fstrict-bool %s 2>&1 | FileCheck %s --check-prefix=CHECK-STRICT
+
+// RUN: %clang -### -mkernel %s 2>&1 | FileCheck %s --check-prefix=CHECK-TRUNCATE
+// RUN: %clang -### -fapple-kext %s 2>&1 | FileCheck %s --check-prefix=CHECK-TRUNCATE
+// RUN: %clang -### -mkernel -fstrict-bool %s 2>&1 | FileCheck %s --check-prefix=CHECK-STRICT
+// RUN: %clang -### -fstrict-bool -mkernel %s 2>&1 | FileCheck %s --check-prefix=CHECK-STRICT
+
+// RUN: not %clang -### -fno-strict-bool=ow-ouch %s 2>&1 | FileCheck %s --check-prefix=CHECK-INVALID
+
+// CHECK-NONE-NOT: -load-bool-from-mem
+// CHECK-STRICT: -load-bool-from-mem=strict
+// CHECK-NONSTRICT: -load-bool-from-mem=nonstrict
+// CHECK-TRUNCATE: -load-bool-from-mem=truncate
+// CHECK-NONZERO: -load-bool-from-mem=nonzero
+// CHECK-INVALID: invalid value 'ow-ouch' in '-fno-strict-bool=ow-ouch'
|
clang/lib/CodeGen/CGExpr.cpp
Outdated
| if (CGM.getCodeGenOpts().getLoadBoolFromMem() == CodeGenOptions::NonZero) { | ||
| auto *NonZero = Builder.CreateICmpNE( | ||
| Value, llvm::Constant::getNullValue(Value->getType()), "loadedv.nz"); | ||
| return Builder.CreateIntCast(NonZero, ResTy, false, "loadedv"); | ||
| } else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't BoolFromMem value NonZero only supposed to be used for boolean types here, not for BitInt etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes.
erichkeane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a 'request changes' since the RFC doesn't seem to have an 'accept' yet from the area team. I'm not sure what the 'range' stuff is doing so we need a better reviewer for that (@efriedma-quic or @rjmccall ).
clang/docs/UsersManual.rst
Outdated
| memory to have any other bit pattern than 0 or 1. This creates some | ||
| optimization opportunities for the compiler, but it enables memory | ||
| corruption if that assumption is violated, for instance if any other value | ||
| is ``memcpy``ed over a ``bool``. This is enabled by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"enabled by default" is something we need to make sure gets out of the RFC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure there's no misunderstanding, -fstrict-bool being enabled by default maintains the status quo. -fno-strict-bool (in either mode) is the change over today's behavior. The name was chosen to align with -fstrict-enums, even though we're looking at an optimization that was always on and that can now be disabled.
| }; | ||
|
|
||
| enum BoolFromMem { | ||
| Strict, ///< In-memory bool values are assumed to be 0 or 1, and any other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should be a scoped enum, else these names are in the outer name space. These names are way too generic to be in a normal enum (that is, either make this an enum class, or give these some sort of ugly prefix, VAST preference for the former).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change it. This was my original implementation before noticing the 3 enums before it.
enum FiniteLoopsKind {
Language, // Not specified, use language standard.
Always, // All loops are assumed to be finite.
Never, // No loop is assumed to be finite.
};
enum AssignmentTrackingOpts {
Disabled,
Enabled,
Forced,
};
enum SanitizeDebugTrapReasonKind {
None, ///< Trap Messages are omitted. This offers the smallest debug info
///< size but at the cost of making traps hard to debug.
Basic, ///< Trap Message is fixed per SanitizerKind. Produces smaller debug
///< info than `Detailed` but is not as helpful for debugging.
Detailed, ///< Trap Message includes more context (e.g. the expression being
///< overflowed). This is more helpful for debugging but produces
///< larger debug info than `Basic`.
};
* Fix ``CodeGenFunction::EmitFromMemory`` breaking BitInt with -fno-strict-bool=nonzero. Add a BitInt test invocation with -fno-strict-bool=nonzero. * Add a note above ``getRangeForType`` warning against extending its functionality without introducing -fstrict flags and sanitizer options. * Add UBSan variants of the strict-bool tests showing that UBSan wins.
rjmccall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I don't think my approval constitutes approval from the Area Team either, though.
clang/docs/UsersManual.rst
Outdated
| contain bit patterns 0 or 1. When =truncate is specified, a ``bool`` is | ||
| true if its least significant bit is set, and false otherwise. When =nonzero | ||
| is specified, a ``bool`` is true when any bit is set, and false otherwise. | ||
| The default is =truncate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the = strings in backticks.
Might be worth saying, just to be perfectly clear, that Clang always stores either 0 or 1 regardless of the value of this setting.
clang/lib/CodeGen/CGExpr.cpp
Outdated
| Ty->isExtVectorBoolType()) | ||
| bool IsBitInt = Ty->isBitIntType(); | ||
| bool HasBoolRep = Ty->hasBooleanRepresentation(); | ||
| if (HasBoolRep && !IsBitInt && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since BitInt(1) falls under hasBooleanRepresentation it is also affected by -fno-strict-bool (i.e. range information is suppressed)? It makes sense that it also follows -fno-strict-bool=nonzero then, so I would not exclude that one here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we treat BitInt(1) as a bool for the purposes of loading from memory, then:
- under
-fstrict-bool, all BitInt values are truncated and insensitive to their padding bits - under
-fno-strict-bool=truncate, all BitInt values are truncated and insensitive to their padding bits - under
-fno-strict-bool=nonzero, all BitInt values are truncated and insensitive to their padding bits except forBitInt(1), which considers the value of its entire storage unit
There would be a strong case to make that it's a bug. Since BitInt values never get !range metadata at this time, I think it's better to leave it alone. If/when they do, there can be a -f{,no-}strict-bitint that controls how all BitInt values, including BitInt(1), behave with respect to their padding bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, did I get that wrong? Does BitInt(1) get !range metadata today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BitInt(1) does get range metadata today, yes: https://godbolt.org/z/YPjeP8nzx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is [0,2) range information today for types that have hasBooleanRepresentation() and not isVectorType() (see this line) and you will suppress this range information for all such types in case of -fno-strict-bool, I would suggest that you change the truncate in a non-zero check under exactly those same conditions when -fno-strict-bool=nonzero, does that make sense? This will include BitInt(1).
clang/lib/CodeGen/CGExpr.cpp
Outdated
| CodeGenOptions::BoolFromMem::NonZero) { | ||
| auto *NonZero = Builder.CreateICmpNE( | ||
| Value, llvm::Constant::getNullValue(Value->getType()), "loadedv.nz"); | ||
| return Builder.CreateIntCast(NonZero, ResTy, false, "loadedv"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the int cast needed here? The icmp instruction returns i1 or a vector of i1. Since Ty->hasBoooleanRepresentation it will also be the case that ResTy is i1 or a vector of i1. So there is nothing to cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really think of a situation where it is needed, but it just returns NonZero if there's no cast to do. It felt less hard-codey but I'll remove it.
|
I don't feel qualified to review this patch; however I want to express support for it and in particular the flexible implementation. Sony will be turning this on-by-default [edit: by which I mean |
This PR implements the -fstrict-bool RFC.
boolvalues are stored as i8 in memory, and it is undefined behavior for aboolvalue to be any value other than 0 or 1. Clang exploits this with range metadata:boolload instructions at any optimization level above -O0 are assumed to only have their lowest bit set. This can create memory safety problems when other bits are set, for instance throughmemcpy.This change allows users to configure this behavior in three ways:
-fstrict-boolrepresents the status quo; range metadata is added at levels above -O0 and allows the compiler to assume in-memoryboolvalues are always either 0 or 1.-fno-strict-bool[={truncate|nonzero}]disables range metadata onboolloaded values and offers two ways to interpret the loaded values.truncatemeans the value is true is the least significant bit is 1 and false otherwise;nonzeromeans the value is true if any bit is 1 and false otherwise.The default is
-fstrict-boolto not change the current behavior of Clang. The default behavior of-fno-strict-boolistruncate.Radar-ID: 139397212