[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
fjricci added a comment. Awesome, good to know. Thanks! Repository: rL LLVM https://reviews.llvm.org/D27607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
vsk added a comment. In https://reviews.llvm.org/D27607#901882, @fjricci wrote: > On platforms where `BOOL` == `signed char`, is it actually undefined behavior > (or is it just bad programming practice) to store a value other than 0 or 1 > in your `BOOL`? I can't find any language specs suggesting that it is, and > given that it's just a typedef for a `signed char`, I don't see why it would > be. Treating BOOL as a regular 'signed char' creates bad interactions with bitfields. For example, this code calls panic: struct S { BOOL b1 : 1; }; S s; s.b1 = YES; if (s.b1 != YES) panic(); > If it's not actually undefined behavior, could we make it controllable via a > separate fsanitize switch (like we have for unsigned integer overflow, which > is also potentially bad practice but not actually undefined behavior). The only defined values for BOOL are 'YES' and 'NO' {1, 0}. We've documented that it's UB to load values outside of this range from a BOOL here: https://developer.apple.com/documentation/code_diagnostics/undefined_behavior_sanitizer/invalid_boolean Repository: rL LLVM https://reviews.llvm.org/D27607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
fjricci added a comment. On platforms where `BOOL` == `signed char`, is it actually undefined behavior (or is it just bad programming practice) to store a value other than 0 or 1 in your `BOOL`? I can't find any language specs suggesting that it is, and given that it's just a typedef for a `signed char`, I don't see why it would be. If it's not actually undefined behavior, could we make it controllable via a separate fsanitize switch (like we have for unsigned integer overflow, which is also potentially bad practice but not actually undefined behavior). Repository: rL LLVM https://reviews.llvm.org/D27607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
This revision was automatically updated to reflect the committed changes. Closed by commit rL289290: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1} (authored by vedantk). Changed prior to commit: https://reviews.llvm.org/D27607?vs=80960&id=80969#toc Repository: rL LLVM https://reviews.llvm.org/D27607 Files: cfe/trunk/lib/CodeGen/CGExpr.cpp cfe/trunk/test/CodeGenObjC/ubsan-bool.m Index: cfe/trunk/test/CodeGenObjC/ubsan-bool.m === --- cfe/trunk/test/CodeGenObjC/ubsan-bool.m +++ cfe/trunk/test/CodeGenObjC/ubsan-bool.m @@ -0,0 +1,13 @@ +// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C + +typedef signed char BOOL; + +// SHARED-LABEL: f1 +BOOL f1() { + // OBJC: call void @__ubsan_handle_load_invalid_value + // C-NOT: call void @__ubsan_handle_load_invalid_value + BOOL a = 2; + return a + 1; +} Index: cfe/trunk/lib/CodeGen/CGExpr.cpp === --- cfe/trunk/lib/CodeGen/CGExpr.cpp +++ cfe/trunk/lib/CodeGen/CGExpr.cpp @@ -24,6 +24,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/NSAPI.h" #include "clang/Frontend/CodeGenOptions.h" #include "llvm/ADT/Hashing.h" #include "llvm/ADT/StringExtras.h" @@ -1219,11 +1220,10 @@ static bool getRangeForType(CodeGenFunction &CGF, QualType Ty, llvm::APInt &Min, llvm::APInt &End, -bool StrictEnums) { +bool StrictEnums, bool IsBool) { const EnumType *ET = Ty->getAs(); bool IsRegularCPlusPlusEnum = CGF.getLangOpts().CPlusPlus && StrictEnums && ET && !ET->getDecl()->isFixed(); - bool IsBool = hasBooleanRepresentation(Ty); if (!IsBool && !IsRegularCPlusPlusEnum) return false; @@ -1253,8 +1253,8 @@ llvm::MDNode *CodeGenFunction::getRangeForLoadFromType(QualType Ty) { llvm::APInt Min, End; - if (!getRangeForType(*this, Ty, Min, End, - CGM.getCodeGenOpts().StrictEnums)) + if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums, + hasBooleanRepresentation(Ty))) return nullptr; llvm::MDBuilder MDHelper(getLLVMContext()); @@ -1313,14 +1313,15 @@ false /*ConvertTypeToTag*/); } - bool NeedsBoolCheck = - SanOpts.has(SanitizerKind::Bool) && hasBooleanRepresentation(Ty); + bool IsBool = hasBooleanRepresentation(Ty) || +NSAPI(CGM.getContext()).isObjCBOOLType(Ty); + bool NeedsBoolCheck = SanOpts.has(SanitizerKind::Bool) && IsBool; bool NeedsEnumCheck = SanOpts.has(SanitizerKind::Enum) && Ty->getAs(); if (NeedsBoolCheck || NeedsEnumCheck) { SanitizerScope SanScope(this); llvm::APInt Min, End; -if (getRangeForType(*this, Ty, Min, End, true)) { +if (getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool)) { --End; llvm::Value *Check; if (!Min) Index: cfe/trunk/test/CodeGenObjC/ubsan-bool.m === --- cfe/trunk/test/CodeGenObjC/ubsan-bool.m +++ cfe/trunk/test/CodeGenObjC/ubsan-bool.m @@ -0,0 +1,13 @@ +// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C + +typedef signed char BOOL; + +// SHARED-LABEL: f1 +BOOL f1() { + // OBJC: call void @__ubsan_handle_load_invalid_value + // C-NOT: call void @__ubsan_handle_load_invalid_value + BOOL a = 2; + return a + 1; +} Index: cfe/trunk/lib/CodeGen/CGExpr.cpp === --- cfe/trunk/lib/CodeGen/CGExpr.cpp +++ cfe/trunk/lib/CodeGen/CGExpr.cpp @@ -24,6 +24,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/NSAPI.h" #include "clang/Frontend/CodeGenOptions.h" #include "llvm/ADT/Hashing.h" #include "llvm/ADT/StringExtras.h" @@ -1219,11 +1220,10 @@ static bool getRangeForType(CodeGenFunction &CGF, QualType Ty, llvm::APInt &Min, llvm::APInt &End, -bool StrictEnums) { +
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
zaks.anna accepted this revision. zaks.anna added a reviewer: zaks.anna. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D27607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
vsk updated this revision to Diff 80960. vsk marked 3 inline comments as done. vsk added a comment. - Use NSAPI's 'is-BOOL' predicate. - Simplify test. https://reviews.llvm.org/D27607 Files: lib/CodeGen/CGExpr.cpp test/CodeGenObjC/ubsan-bool.m Index: test/CodeGenObjC/ubsan-bool.m === --- /dev/null +++ test/CodeGenObjC/ubsan-bool.m @@ -0,0 +1,13 @@ +// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C + +typedef signed char BOOL; + +// SHARED-LABEL: f1 +BOOL f1() { + // OBJC: call void @__ubsan_handle_load_invalid_value + // C-NOT: call void @__ubsan_handle_load_invalid_value + BOOL a = 2; + return a + 1; +} Index: lib/CodeGen/CGExpr.cpp === --- lib/CodeGen/CGExpr.cpp +++ lib/CodeGen/CGExpr.cpp @@ -24,6 +24,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/NSAPI.h" #include "clang/Frontend/CodeGenOptions.h" #include "llvm/ADT/Hashing.h" #include "llvm/ADT/StringExtras.h" @@ -1219,11 +1220,10 @@ static bool getRangeForType(CodeGenFunction &CGF, QualType Ty, llvm::APInt &Min, llvm::APInt &End, -bool StrictEnums) { +bool StrictEnums, bool IsBool) { const EnumType *ET = Ty->getAs(); bool IsRegularCPlusPlusEnum = CGF.getLangOpts().CPlusPlus && StrictEnums && ET && !ET->getDecl()->isFixed(); - bool IsBool = hasBooleanRepresentation(Ty); if (!IsBool && !IsRegularCPlusPlusEnum) return false; @@ -1253,8 +1253,8 @@ llvm::MDNode *CodeGenFunction::getRangeForLoadFromType(QualType Ty) { llvm::APInt Min, End; - if (!getRangeForType(*this, Ty, Min, End, - CGM.getCodeGenOpts().StrictEnums)) + if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums, + hasBooleanRepresentation(Ty))) return nullptr; llvm::MDBuilder MDHelper(getLLVMContext()); @@ -1313,14 +1313,15 @@ false /*ConvertTypeToTag*/); } - bool NeedsBoolCheck = - SanOpts.has(SanitizerKind::Bool) && hasBooleanRepresentation(Ty); + bool IsBool = hasBooleanRepresentation(Ty) || +NSAPI(CGM.getContext()).isObjCBOOLType(Ty); + bool NeedsBoolCheck = SanOpts.has(SanitizerKind::Bool) && IsBool; bool NeedsEnumCheck = SanOpts.has(SanitizerKind::Enum) && Ty->getAs(); if (NeedsBoolCheck || NeedsEnumCheck) { SanitizerScope SanScope(this); llvm::APInt Min, End; -if (getRangeForType(*this, Ty, Min, End, true)) { +if (getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool)) { --End; llvm::Value *Check; if (!Min) Index: test/CodeGenObjC/ubsan-bool.m === --- /dev/null +++ test/CodeGenObjC/ubsan-bool.m @@ -0,0 +1,13 @@ +// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C + +typedef signed char BOOL; + +// SHARED-LABEL: f1 +BOOL f1() { + // OBJC: call void @__ubsan_handle_load_invalid_value + // C-NOT: call void @__ubsan_handle_load_invalid_value + BOOL a = 2; + return a + 1; +} Index: lib/CodeGen/CGExpr.cpp === --- lib/CodeGen/CGExpr.cpp +++ lib/CodeGen/CGExpr.cpp @@ -24,6 +24,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/NSAPI.h" #include "clang/Frontend/CodeGenOptions.h" #include "llvm/ADT/Hashing.h" #include "llvm/ADT/StringExtras.h" @@ -1219,11 +1220,10 @@ static bool getRangeForType(CodeGenFunction &CGF, QualType Ty, llvm::APInt &Min, llvm::APInt &End, -bool StrictEnums) { +bool StrictEnums, bool IsBool) { const EnumType *ET = Ty->getAs(); bool IsRegularCPlusPlusEnum = CGF.getLangOpts().CPlusPlus && StrictEnums && ET && !ET->getDecl()->isFixed(); - bool IsBool = hasBooleanRepresentation(Ty); if (!IsBool && !IsRegularCPlusPlusEnum) r
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
vsk marked 3 inline comments as done. vsk added a comment. Thanks for your feedback. I will updated the patch shortly. Comment at: lib/CodeGen/CGExpr.cpp:1221 +/// Check if \p Ty is defined as BOOL in a system header. In ObjC language +/// modes, it's safe to treat such a type as 'the builtin bool'. +static bool isObjCBool(QualType Ty, const SourceManager &SM, ahatanak wrote: > If your intention is to exclude BOOLs defined in files that aren't system > headers, is it possible to add a test for that? > > > ``` > void foo() { > typedef long long BOOL; > ... > } > ``` Thinking about this some more, I think it's enough to check for an ObjC language mode, and that the system header check is unnecessary. Comment at: lib/CodeGen/CGExpr.cpp:1222 +/// modes, it's safe to treat such a type as 'the builtin bool'. +static bool isObjCBool(QualType Ty, const SourceManager &SM, + const LangOptions &LO) { zaks.anna wrote: > Could you use the existing method for this? From NSAPI.h: > > ``` >// \brief Returns true if \param T is a typedef of "BOOL" in objective-c. >bool isObjCBOOLType(QualType T) const; > > ``` > Thanks, this is a lot better than rolling my own. Comment at: lib/CodeGen/CGExpr.cpp:1224 + const LangOptions &LO) { + if (!LO.ObjC1 && !LO.ObjC2) +return false; arphaman wrote: > LangOptions.ObjC1 should be always set if LangOptions.ObjC2 is set, so the > second check is redundant I think. Using NSAPI takes care of this. https://reviews.llvm.org/D27607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
ahatanak added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1221 +/// Check if \p Ty is defined as BOOL in a system header. In ObjC language +/// modes, it's safe to treat such a type as 'the builtin bool'. +static bool isObjCBool(QualType Ty, const SourceManager &SM, If your intention is to exclude BOOLs defined in files that aren't system headers, is it possible to add a test for that? ``` void foo() { typedef long long BOOL; ... } ``` https://reviews.llvm.org/D27607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
zaks.anna added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1222 +/// modes, it's safe to treat such a type as 'the builtin bool'. +static bool isObjCBool(QualType Ty, const SourceManager &SM, + const LangOptions &LO) { Could you use the existing method for this? From NSAPI.h: ``` // \brief Returns true if \param T is a typedef of "BOOL" in objective-c. bool isObjCBOOLType(QualType T) const; ``` https://reviews.llvm.org/D27607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
arphaman added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1224 + const LangOptions &LO) { + if (!LO.ObjC1 && !LO.ObjC2) +return false; LangOptions.ObjC1 should be always set if LangOptions.ObjC2 is set, so the second check is redundant I think. https://reviews.llvm.org/D27607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27607: [ubsan] Treat ObjC's BOOL as if its range is always {0, 1}
vsk created this revision. vsk added a reviewer: ahatanak. vsk added subscribers: kubabrecka, cfe-commits. On some Apple platforms, the ObjC BOOL type is defined as a signed char. When performing instrumentation for -fsanitize=bool, we'd like to treat the range of BOOL like it's always {0, 1}. While we can't change clang's IRGen for char-backed BOOL's due to ABI compatibility concerns, we can teach ubsan to catch potential abuses of this type. rdar://problem/29502773 https://reviews.llvm.org/D27607 Files: lib/CodeGen/CGExpr.cpp test/CodeGenObjC/ubsan-bool.m Index: test/CodeGenObjC/ubsan-bool.m === --- /dev/null +++ test/CodeGenObjC/ubsan-bool.m @@ -0,0 +1,25 @@ +// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C + +#ifdef IS_SYSHEADER + +// Create a system definition of ObjC's BOOL. +#pragma clang system_header +typedef signed char BOOL; + +#else + +// Import an official-looking definition of BOOL. +#define IS_SYSHEADER +#include __FILE__ + +// SHARED-LABEL: f1 +BOOL f1() { + // OBJC: call void @__ubsan_handle_load_invalid_value + // C-NOT: call void @__ubsan_handle_load_invalid_value + BOOL a = 2; + return a + 1; +} + +#endif // IS_SYSHEADER Index: lib/CodeGen/CGExpr.cpp === --- lib/CodeGen/CGExpr.cpp +++ lib/CodeGen/CGExpr.cpp @@ -1217,13 +1217,29 @@ return false; } +/// Check if \p Ty is defined as BOOL in a system header. In ObjC language +/// modes, it's safe to treat such a type as 'the builtin bool'. +static bool isObjCBool(QualType Ty, const SourceManager &SM, + const LangOptions &LO) { + if (!LO.ObjC1 && !LO.ObjC2) +return false; + + const auto *TT = Ty.getTypePtr()->getAs(); + if (!TT) +return false; + + const TypedefNameDecl *TND = TT->getDecl(); + return TND->getName() == "BOOL" && + SM.isInSystemHeader( + TND->getTypeSourceInfo()->getTypeLoc().getBeginLoc()); +} + static bool getRangeForType(CodeGenFunction &CGF, QualType Ty, llvm::APInt &Min, llvm::APInt &End, -bool StrictEnums) { +bool StrictEnums, bool IsBool) { const EnumType *ET = Ty->getAs(); bool IsRegularCPlusPlusEnum = CGF.getLangOpts().CPlusPlus && StrictEnums && ET && !ET->getDecl()->isFixed(); - bool IsBool = hasBooleanRepresentation(Ty); if (!IsBool && !IsRegularCPlusPlusEnum) return false; @@ -1253,8 +1269,8 @@ llvm::MDNode *CodeGenFunction::getRangeForLoadFromType(QualType Ty) { llvm::APInt Min, End; - if (!getRangeForType(*this, Ty, Min, End, - CGM.getCodeGenOpts().StrictEnums)) + if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums, + hasBooleanRepresentation(Ty))) return nullptr; llvm::MDBuilder MDHelper(getLLVMContext()); @@ -1313,14 +1329,16 @@ false /*ConvertTypeToTag*/); } - bool NeedsBoolCheck = - SanOpts.has(SanitizerKind::Bool) && hasBooleanRepresentation(Ty); + bool IsBool = + hasBooleanRepresentation(Ty) || + isObjCBool(Ty, CGM.getContext().getSourceManager(), CGM.getLangOpts()); + bool NeedsBoolCheck = SanOpts.has(SanitizerKind::Bool) && IsBool; bool NeedsEnumCheck = SanOpts.has(SanitizerKind::Enum) && Ty->getAs(); if (NeedsBoolCheck || NeedsEnumCheck) { SanitizerScope SanScope(this); llvm::APInt Min, End; -if (getRangeForType(*this, Ty, Min, End, true)) { +if (getRangeForType(*this, Ty, Min, End, /*StrictEnums=*/true, IsBool)) { --End; llvm::Value *Check; if (!Min) Index: test/CodeGenObjC/ubsan-bool.m === --- /dev/null +++ test/CodeGenObjC/ubsan-bool.m @@ -0,0 +1,25 @@ +// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,OBJC +// RUN: %clang_cc1 -x c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=bool %s -o - | FileCheck %s -check-prefixes=SHARED,C + +#ifdef IS_SYSHEADER + +// Create a system definition of ObjC's BOOL. +#pragma clang system_header +typedef signed char BOOL; + +#else + +// Import an official-looking definition of BOOL. +#define IS_SYSHEADER +#include __FILE__ + +// SHARED-LABEL: f1 +