[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137
+ overflow happens (signed or unsigned).
+ Both of these two issues are handled by ``-fsanitize=implicit-conversion``
+ group of checks.
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable

lebedev.ri wrote:
> rsmith wrote:
> > I don't think that's true (not until you add a sanitizer for signed <-> 
> > unsigned conversions that change the value). `4U / -2` produces the 
> > unexpected result `0U` rather than the mathematically-correct result `-2`, 
> > and `-fsanitize=implicit-conversion` doesn't catch it. Maybe just strike 
> > this sentence for now?
> > 
> > In fact... I think this is too much text to be adding to this bulleted 
> > list, which is just supposed to summarize the available checks. Maybe 
> > replace the description with
> > 
> > Signed integer overflow, where the result of a signed integer 
> > computation cannot be represented in its type. This includes all the checks 
> > covered by ``-ftrapv``, as well as checks for signed division overflow 
> > (``INT_MIN/-1``), but not checks for lossy implicit conversions performed 
> > before the computation (see ``-fsanitize=implicit-conversion``).
> I will assume you meant "lossy implicit conversions performed *after* the 
> computation".
I really meant "performed before", for cases like `4u / -2`, where `-2` is 
implicitly converted to `UINT_MAX - 2` before the computation. Conversions that 
are performed after a computation aren't part of the computation at all, so I 
think it's much clearer that they're not in scope for this sanitizer.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137
+ overflow happens (signed or unsigned).
+ Both of these two issues are handled by ``-fsanitize=implicit-conversion``
+ group of checks.
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable

rsmith wrote:
> lebedev.ri wrote:
> > rsmith wrote:
> > > I don't think that's true (not until you add a sanitizer for signed <-> 
> > > unsigned conversions that change the value). `4U / -2` produces the 
> > > unexpected result `0U` rather than the mathematically-correct result 
> > > `-2`, and `-fsanitize=implicit-conversion` doesn't catch it. Maybe just 
> > > strike this sentence for now?
> > > 
> > > In fact... I think this is too much text to be adding to this bulleted 
> > > list, which is just supposed to summarize the available checks. Maybe 
> > > replace the description with
> > > 
> > > Signed integer overflow, where the result of a signed integer 
> > > computation cannot be represented in its type. This includes all the 
> > > checks covered by ``-ftrapv``, as well as checks for signed division 
> > > overflow (``INT_MIN/-1``), but not checks for lossy implicit conversions 
> > > performed before the computation (see ``-fsanitize=implicit-conversion``).
> > I will assume you meant "lossy implicit conversions performed *after* the 
> > computation".
> I really meant "performed before", for cases like `4u / -2`, where `-2` is 
> implicitly converted to `UINT_MAX - 2` before the computation. Conversions 
> that are performed after a computation aren't part of the computation at all, 
> so I think it's much clearer that they're not in scope for this sanitizer.
Ok, with that additional explanation, i do see the error of my ways, and will 
re-adjust the docs accordingly.
Sorry.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC338288: [clang][ubsan] Implicit Conversion Sanitizer - 
integer truncation  - clang part (authored by lebedevri, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: include/clang/Basic/Sanitizers.h
===
--- include/clang/Basic/Sanitizers.h
+++ include/clang/Basic/Sanitizers.h
@@ -84,7 +84,8 @@
 /// Return the sanitizers which do not affect preprocessing.
 inline SanitizerMask getPPTransparentSanitizers() {
   return SanitizerKind::CFI | SanitizerKind::Integer |
- SanitizerKind::Nullability | SanitizerKind::Undefined;
+ SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
+ SanitizerKind::Undefined;
 }
 
 } // namespace clang
Index: include/clang/Basic/Sanitizers.def
===
--- include/clang/Basic/Sanitizers.def
+++ include/clang/Basic/Sanitizers.def
@@ -131,9 +131,14 @@
 // -fsanitize=undefined-trap is an alias for -fsanitize=undefined.
 SANITIZER_GROUP("undefined-trap", UndefinedTrap, Undefined)
 
+// ImplicitConversionSanitizer
+SANITIZER("implicit-integer-truncation", ImplicitIntegerTruncation)
+SANITIZER_GROUP("implicit-conversion", ImplicitConversion,
+ImplicitIntegerTruncation)
+
 SANITIZER_GROUP("integer", Integer,
-SignedIntegerOverflow | UnsignedIntegerOverflow | Shift |
-IntegerDivideByZero)
+ImplicitIntegerTruncation | IntegerDivideByZero | Shift |
+SignedIntegerOverflow | UnsignedIntegerOverflow)
 
 SANITIZER("local-bounds", LocalBounds)
 SANITIZER_GROUP("bounds", Bounds, ArrayBounds | LocalBounds)
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -29,7 +29,22 @@
 // CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-integer-truncation),?){6}"}}
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fno-sanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-trap=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-TRAP
+// CHECK-implicit-conversion: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
 
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
Index: 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137
+ overflow happens (signed or unsigned).
+ Both of these two issues are handled by ``-fsanitize=implicit-conversion``
+ group of checks.
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable

rsmith wrote:
> I don't think that's true (not until you add a sanitizer for signed <-> 
> unsigned conversions that change the value). `4U / -2` produces the 
> unexpected result `0U` rather than the mathematically-correct result `-2`, 
> and `-fsanitize=implicit-conversion` doesn't catch it. Maybe just strike this 
> sentence for now?
> 
> In fact... I think this is too much text to be adding to this bulleted list, 
> which is just supposed to summarize the available checks. Maybe replace the 
> description with
> 
> Signed integer overflow, where the result of a signed integer computation 
> cannot be represented in its type. This includes all the checks covered by 
> ``-ftrapv``, as well as checks for signed division overflow (``INT_MIN/-1``), 
> but not checks for lossy implicit conversions performed before the 
> computation (see ``-fsanitize=implicit-conversion``).
I will assume you meant "lossy implicit conversions performed *after* the 
computation".


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 158030.
lebedev.ri marked 9 inline comments as done.
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.

Address last portion of @rsmith review notes.

@rsmith, @vsk, @erichkeane - thank you for the review!


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -29,7 +29,22 @@
 // CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-integer-truncation),?){6}"}}
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fno-sanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-trap=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-TRAP
+// CHECK-implicit-conversion: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
 
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,256 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit conversion
+// == //
+// These contain one implicit truncating conversion, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit conversion.
+

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.

Only comments on documentation and assertions. Feel free to commit once these 
are addressed to your satisfaction.




Comment at: docs/ReleaseNotes.rst:310
+
+  Just like ``-fsanitize=integer``, these issues are **not** undefined
+  behaviour. But they are not *always* intentional, and are somewhat hard to

"Just like -fsanitize=integer" -> "Just like other -fsanitize=integer checks", 
now that this is part of  `-fsanitize=integer`.



Comment at: docs/UndefinedBehaviorSanitizer.rst:17
 * Signed integer overflow
+* Problematic Implicit Casts (not UB, but not always intentional)
 * Conversion to, from, or between floating-point types which would

rsmith wrote:
> Don't use Title Caps here. "Problematic implicit conversions"
I don't think it makes sense to list this here, as it's not undefined behavior, 
and this is a list of undefined behavior that UBSan catches. (And I think it 
makes sense from a communication perspective to consider the non-UB checks to 
be "not part of UBSan but handled by the same infrastructure".)



Comment at: docs/UndefinedBehaviorSanitizer.rst:97
+ width, is not equal to the original value before the downcast.
+ This issue may often be caused by an implicit integer conversions.
   -  ``-fsanitize=integer-divide-by-zero``: Integer division by zero.

I don't think this last sentence adds anything, and it creates the impression 
that the issue is sometimes caused by something other than implicit integer 
conversions (which it isn't, as far as I can tell). Maybe just delete the last 
sentence here?

And instead, something like this would be useful:

"Issues caught by this sanitizer are not undefined behavior, but are often 
unintentional."



Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137
+ overflow happens (signed or unsigned).
+ Both of these two issues are handled by ``-fsanitize=implicit-conversion``
+ group of checks.
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable

I don't think that's true (not until you add a sanitizer for signed <-> 
unsigned conversions that change the value). `4U / -2` produces the unexpected 
result `0U` rather than the mathematically-correct result `-2`, and 
`-fsanitize=implicit-conversion` doesn't catch it. Maybe just strike this 
sentence for now?

In fact... I think this is too much text to be adding to this bulleted list, 
which is just supposed to summarize the available checks. Maybe replace the 
description with

Signed integer overflow, where the result of a signed integer computation 
cannot be represented in its type. This includes all the checks covered by 
``-ftrapv``, as well as checks for signed division overflow (``INT_MIN/-1``), 
but not checks for lossy implicit conversions performed before the computation 
(see ``-fsanitize=implicit-conversion``).



Comment at: docs/UndefinedBehaviorSanitizer.rst:140-148
   -  ``-fsanitize=unsigned-integer-overflow``: Unsigned integer
  overflows. Note that unlike signed integer overflow, unsigned integer
  is not undefined behavior. However, while it has well-defined semantics,
- it is often unintentional, so UBSan offers to catch it.
+ it is often unintentional, so UBSan offers to catch it. Note that this
+ check does not diagnose lossy implicit integer conversions. Also note that
+ integer conversions may result in an unexpected computation result, even
+ though no overflow happens (signed or unsigned).

And here something like:

Unsigned integer overflow, where the result of an unsigned integer 
computation cannot be represented in its type. Unlike signed integer overflow, 
this is not undefined behavior, but it is often unintentional. This sanitizer 
does not check for lossy implicit conversions performed before such a 
computation (see ``-fsanitize=implicit-conversion``).



Comment at: docs/UndefinedBehaviorSanitizer.rst:165
  behavior (e.g. unsigned integer overflow).
+ Enables ``-fsanitize=implicit-integer-truncation``.
+  -  ``-fsanitize=implicit-conversion``: Checks for suspicious behaviours of

If we're going to list which sanitizers are enabled here, we should list them 
all:

Enables ``signed-integer-overflow``, ``unsigned-integer-overflow``, 
``shift``, ``integer-divide-by-zero``, and ``implicit-integer-truncation``.



Comment at: docs/UndefinedBehaviorSanitizer.rst:95
+ of bigger bit width to smaller bit width, if that results in data loss.
+ That is, if the demoted value, after casting back to the original width,
+ is not equal to the original value before the downcast. This issue may

lebedev.ri wrote:
> rsmith wrote:
> > rsmith wrote:
> > > erichkeane wrote:
> > > > I think the last 2 commas in 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Oops, forgot to submit the inline comments.
(It is inconvenient that they aren't submitted with the rest.)




Comment at: docs/ReleaseNotes.rst:292
+  store = store + 768; // before addition, 'store' was promoted to int.
+(void)consume((unsigned int)val); // OK, the truncation is implicit.
+  }

rsmith wrote:
> implicit -> explicit
Whoops.



Comment at: docs/UndefinedBehaviorSanitizer.rst:95
+ of bigger bit width to smaller bit width, if that results in data loss.
+ That is, if the demoted value, after casting back to the original width,
+ is not equal to the original value before the downcast. This issue may

rsmith wrote:
> rsmith wrote:
> > erichkeane wrote:
> > > I think the last 2 commas in this sentence are unnecessary?  
> > I would parenthesize the "where the demoted value [...] would have a 
> > different value from the original" clause, since it's just explaining what 
> > we mean by "data loss".
> Is this really the right rule, though? Consider:
> 
> ```
> unsigned int x = 0x81234567;
> int y = x; // does the sanitizer catch this or not?
> ```
> 
> Here, the value of `x` is not the same as the value of `y` (assuming 32-bit 
> int): `y` is negative. But this is not "data loss" according to the 
> documented meaning of the sanitizer. I think we should produce a sanitizer 
> trap on this case.
I've reverted this to my original text.
It should now convey the correct idea, but i'm not sure this is correct English.

> unsigned int x = 0x81234567;
> int y = x; // does the sanitizer catch this or not?
No, it does not. It indeed should. I do plan on following-up with that,
thus i've adding the group (``-fsanitize=implicit-conversion``), not just one 
check.



Comment at: include/clang/Basic/Sanitizers.def:134-136
 SANITIZER_GROUP("integer", Integer,
 SignedIntegerOverflow | UnsignedIntegerOverflow | Shift |
 IntegerDivideByZero)

rsmith wrote:
> `-fsanitize=integer` should include `-fsanitize=implicit-integer-truncation`.
Wow. Ok.



Comment at: lib/CodeGen/CGExprScalar.cpp:954-955
+  // We only care about int->int casts here, and ignore casts to/from pointer.
+  if (!(isa(SrcTy) && isa(DstTy)))
+return;
+  // We do not care about booleans.

rsmith wrote:
> Check the Clang types here, not the LLVM types. There is no guarantee that 
> only integer types get converted to LLVM `IntegerType`s. (But if you like, 
> you can assert that `SrcTy` and `DstTy` are `IntegerType`s after checking 
> that the clang types are both integer types.)
Interesting, ok.



Comment at: lib/CodeGen/CGExprScalar.cpp:956-958
+  // We do not care about booleans.
+  if (SrcType->isBooleanType() || DstType->isBooleanType())
+return;

rsmith wrote:
> I believe this is redundant: we don't get here for an integer to boolean 
> conversion, and a boolean to integer conversion would always be caught by the 
> bit width check below.
Uhm, i'll replace it with an assert then.



Comment at: lib/CodeGen/CGExprScalar.cpp:962-964
+  // This must be truncation. Else we do not care.
+  if (SrcBits <= DstBits)
+return;

rsmith wrote:
> I think you should also catch casts that change signedness in the case if the 
> sign bit is set on the value. (Though if you want to defer this to a 
> follow-up change and maybe give the sanitizer a different name, that's fine 
> with me.)
Yes, thank you for bringing this up.
That is certainly the plain, but i always planned to add that later on.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 157665.
lebedev.ri marked 9 inline comments as done and 2 inline comments as done.
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.

Hopefully address @rsmith review notes:

- s/cast/conversion/ where appropriate
- Some wording in docs
- Some 'legality' checks in `ScalarExprEmitter::EmitIntegerTruncationCheck()`.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -29,7 +29,22 @@
 // CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-integer-truncation),?){6}"}}
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fno-sanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-trap=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-TRAP
+// CHECK-implicit-conversion: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-implicit-conversion-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
 
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,256 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit conversion
+// == //
+// These contain one implicit truncating 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: docs/ReleaseNotes.rst:49-51
+- A new Implicit Cast Sanitizer (``-fsanitize=implicit-cast``) group was added.
+  Please refer to the :ref:`release-notes-ubsan` section of the release notes
+  for the details.

Regarding the name of this sanitizer: C and C++ refer to these as "implicit 
conversions" not "implicit casts", and "implicit cast" is a contradiction in 
terms -- a cast is explicit syntax for performing a conversion. We should use 
the external terminology here ("implicit-conversion") rather than the 
slightly-odd clang-specific convention of calling an implicit conversion an 
"implicit cast".



Comment at: docs/ReleaseNotes.rst:290
+  void test(unsigned long val) {
+if (consume(val)) // the value got silently truncated.
+  store = store + 768; // before addition, 'store' was promoted to int.

got -> may have been



Comment at: docs/ReleaseNotes.rst:292
+  store = store + 768; // before addition, 'store' was promoted to int.
+(void)consume((unsigned int)val); // OK, the truncation is implicit.
+  }

implicit -> explicit



Comment at: docs/UndefinedBehaviorSanitizer.rst:95
+ of bigger bit width to smaller bit width, if that results in data loss.
+ That is, if the demoted value, after casting back to the original width,
+ is not equal to the original value before the downcast. This issue may

erichkeane wrote:
> I think the last 2 commas in this sentence are unnecessary?  
I would parenthesize the "where the demoted value [...] would have a different 
value from the original" clause, since it's just explaining what we mean by 
"data loss".



Comment at: docs/UndefinedBehaviorSanitizer.rst:95
+ of bigger bit width to smaller bit width, if that results in data loss.
+ That is, if the demoted value, after casting back to the original width,
+ is not equal to the original value before the downcast. This issue may

rsmith wrote:
> erichkeane wrote:
> > I think the last 2 commas in this sentence are unnecessary?  
> I would parenthesize the "where the demoted value [...] would have a 
> different value from the original" clause, since it's just explaining what we 
> mean by "data loss".
Is this really the right rule, though? Consider:

```
unsigned int x = 0x81234567;
int y = x; // does the sanitizer catch this or not?
```

Here, the value of `x` is not the same as the value of `y` (assuming 32-bit 
int): `y` is negative. But this is not "data loss" according to the documented 
meaning of the sanitizer. I think we should produce a sanitizer trap on this 
case.



Comment at: docs/UndefinedBehaviorSanitizer.rst:17
 * Signed integer overflow
+* Problematic Implicit Casts (not UB, but not always intentional)
 * Conversion to, from, or between floating-point types which would

Don't use Title Caps here. "Problematic implicit conversions"



Comment at: docs/UndefinedBehaviorSanitizer.rst:131-133
+ overflow in signed division (``INT_MIN / -1``). Please do note that this
+ check does not diagnose lossy implicit integer conversions. Also please be
+ aware of integer conversions, as those may result in an unexpected

Remove the "Please"s here. We don't need to beg the reader to read the rest of 
the sentence. Just "Note that this [...]. Also note that integer conversions 
may result in an unexpected computation result, [...]"



Comment at: docs/UndefinedBehaviorSanitizer.rst:142-147
+ it is often unintentional, so UBSan offers to catch it. Please do note 
that
+ this check does not diagnose lossy implicit integer conversions. Also
+ please be aware of integer conversions, as those may result in an
+ unexpected computation results, even though no overflow happens (signed or
+ unsigned). Both of these two issues are handled by 
``-fsanitize=implicit-cast``
+ group of checks.

Likewise here.



Comment at: include/clang/Basic/Sanitizers.def:134-136
 SANITIZER_GROUP("integer", Integer,
 SignedIntegerOverflow | UnsignedIntegerOverflow | Shift |
 IntegerDivideByZero)

`-fsanitize=integer` should include `-fsanitize=implicit-integer-truncation`.



Comment at: lib/CodeGen/CGExprScalar.cpp:954-955
+  // We only care about int->int casts here, and ignore casts to/from pointer.
+  if (!(isa(SrcTy) && isa(DstTy)))
+return;
+  // We do not care about booleans.

Check the Clang types here, not the LLVM types. There is no guarantee that only 
integer types get converted to LLVM `IntegerType`s. (But if you like, you can 
assert that `SrcTy` and `DstTy` are `IntegerType`s after checking that the 
clang types are both integer types.)

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 157585.
lebedev.ri added a comment.

Rebase,
Address @rsmith review notes - just inline https://reviews.llvm.org/D49844.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,256 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned short)c;
+}
+
+// Implicit truncation before explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference1
+unsigned char explicit_cast_interference1(unsigned int c) {
+  // CHECK-SANITIZE: 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: docs/UndefinedBehaviorSanitizer.rst:93-97
+  -  ``-fsanitize=implicit-integer-truncation``: Implicit cast from integer
+ of bigger bit width to smaller bit width, if that results in data loss.
+ That is, if the demoted value, after casting back to the original width,
+ is not equal to the original value before the downcast. This issue may
+ often be caused by an implicit integer conversions.

aaron.ballman wrote:
> How about: `Implicit cast from a value of integral type which results in data 
> loss where the demoted value, when cast back to the original type, would have 
> a different value than the original. This issue may be caused by an implicit 
> conversion.`
Thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 157498.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

Small rewording in `docs/UndefinedBehaviorSanitizer.rst` thanks to @erichkeane 
& @aaron.ballman!


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,256 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned short)c;
+}
+
+// Implicit truncation before explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference1
+unsigned char 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: docs/UndefinedBehaviorSanitizer.rst:93-97
+  -  ``-fsanitize=implicit-integer-truncation``: Implicit cast from integer
+ of bigger bit width to smaller bit width, if that results in data loss.
+ That is, if the demoted value, after casting back to the original width,
+ is not equal to the original value before the downcast. This issue may
+ often be caused by an implicit integer conversions.

How about: `Implicit cast from a value of integral type which results in data 
loss where the demoted value, when cast back to the original type, would have a 
different value than the original. This issue may be caused by an implicit 
conversion.`


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.

1 Nit, otherwise LGTM.




Comment at: docs/UndefinedBehaviorSanitizer.rst:95
+ of bigger bit width to smaller bit width, if that results in data loss.
+ That is, if the demoted value, after casting back to the original width,
+ is not equal to the original value before the downcast. This issue may

I think the last 2 commas in this sentence are unnecessary?  


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: test/CodeGenCXX/catch-implicit-integer-truncations.cpp:8-33
+// == 
//
+// Check that explicit cast does not interfere with implicit cast
+// == 
//
+// These contain one implicit truncating cast, and one explicit truncating 
cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.

@rsmith these tests //should// be equivalent to what you have brought up, so 
that situation was already tested.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 157458.
lebedev.ri marked an inline comment as done.
lebedev.ri added a reviewer: erichkeane.
lebedev.ri added a subscriber: erichkeane.
lebedev.ri added a comment.

Address @rsmith & @erichkeane [IRC] review notes:

- https://reviews.llvm.org/D49838 - [AST] Sink 'part of explicit cast' down 
into ImplicitCastExpr
- https://reviews.llvm.org/D49844 - [AST] Add a isActuallyImplicitCast() helper 
to the CastExpr class.
- Drop no longer needed `CastExprStackGuard`, 
`ScalarExprEmitter::IsTopCastPartOfExplictCast()`, just use 
`CastExpr::isActuallyImplicitCast()` directly.

This should be a NFC change, there should not be any functionality change 
because of this.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,256 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:979-1003
+bool ScalarExprEmitter::IsTopCastPartOfExplictCast() {
+  assert(!CastExprStack.empty());
+  // Walk the current stack of CastExprs in reverse order.
+  // That is, the current CastExpr, which is the top()/back() one,
+  // will be processed first.
+  const CastExpr *PreviousCast = nullptr;
+  for (auto CastRef : llvm::reverse(CastExprStack)) {

Based on IRC disscussion with @rsmith, it seems this should be just `return 
!Cast->getIsPartOfExplicitCast();` (and inline it), and no need for the 
`CastExprStack` and stuff.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D48958#1173860, @vsk wrote:

> LGTM, although I think it'd be helpful to have another +1 just to be safe.


Thank you for the review!
It would indeed be great if someone else could take a look, especially since we 
are **so** close to the branching point.

In https://reviews.llvm.org/D48958#1173860, @vsk wrote:

> ...




In https://reviews.llvm.org/D48958#1173860, @vsk wrote:

> These four at least don't look like false positives:
>
> - Maybe we should consider special-casing assignments of "-1" to unsigned 
> values? This seems somewhat idiomatic.


I **personally** would use `~0U` there.
One more datapoint: the `implicit-sign-change` will/should still complain about 
that case.
So **personally** i'd like to keep it.

> - At least a few of these are due to not being explicit about dropping the 
> high bits of hash_combine()'s result. Given that this check is opt-in, that 
> that seems like a legitimate diagnostic (lost entropy).
> - The TargetLoweringBase.cpp diagnostic looks a bit scary.




Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

LGTM, although I think it'd be helpful to have another +1 just to be safe.

I did two small experiments with this using a Stage1 Rel+Asserts compiler:

Stage2 Rel+Asserts build of llvm-tblgen:
ninja llvm-tblgen  384.27s user 14.98s system 1467% cpu 27.203 total

Stage2 Rel+Asserts build of llvm-tblgen with implicit-cast checking:
ninja llvm-tblgen  385.15s user 15.02s system 1472% cpu 27.170 total

With caveats about having a small sample size here and testing with an 
asserts-enabled stage1 build, I don't see any red flags about the compile-time 
overhead of the new check. I would have liked to measure the check against more 
code, but I couldn't complete a stage2 build due to a diagnostic which might 
plausibly point to a real issue in tblgen:

  
/Users/vsk/src/llvm.org-lldbsan/llvm/utils/TableGen/RegisterInfoEmitter.cpp:604:17:
 runtime error: implicit cast from type 'int' of value -1 (32-bit, signed) to 
type 'const unsigned short' changed the value to 65535 (16-bit, unsigned)

With -fno-sanitize-recover=all disabled, I found a few more reports:

  /Users/vsk/src/llvm.org-lldbsan/llvm/include/llvm/Object/Archive.h:278:38: 
runtime error: implicit cast from type 'int' of value -1 (32-bit, signed) to 
type 'uint16_t' (aka 'unsigned short') changed the value to 65535 (16-bit, 
unsigned)
  --> uint16_t FirstRegularStartOfFile = -1;
  
  /Users/vsk/src/llvm.org-lldbsan/llvm/lib/Analysis/MemorySSA.cpp:199:12: 
runtime error: implicit cast from type 'size_t' (aka 'unsigned long') of value 
4969132974595412838 (64-bit, unsigned) to type 'unsigned int' changed the value 
to 3765474150 (32-bit, unsigned)
  --> hash_combine() result casted to unsigned
  
  
/Users/vsk/src/llvm.org-lldbsan/llvm/lib/CodeGen/TargetLoweringBase.cpp:1212:30:
 runtime error: implicit cast from type 'unsigned int' of value 512 (32-bit, 
unsigned) to type 'unsigned char' changed the value to 0 (8-bit, unsigned)
  --> NumRegistersForVT[i] = getVectorTypeBreakdownMVT(...)
  
  
/Users/vsk/src/llvm.org-lldbsan/llvm/lib/Transforms/Scalar/EarlyCSE.cpp:136:12: 
runtime error: implicit cast from type 'size_t' (aka 'unsigned long') of value 
16583795711468875482 (64-bit, unsigned) to type 'unsigned int' changed the 
value to 3116347098 (32-bit, unsigned)
  --> hash_combine() result casted to unsigned
  ...

These four at least don't look like false positives:

- Maybe we should consider special-casing assignments of "-1" to unsigned 
values? This seems somewhat idiomatic.
- At least a few of these are due to not being explicit about dropping the high 
bits of hash_combine()'s result. Given that this check is opt-in, that that 
seems like a legitimate diagnostic (lost entropy).
- The TargetLoweringBase.cpp diagnostic looks a bit scary.




Comment at: lib/CodeGen/CGExprScalar.cpp:986
+  for (auto CastRef : llvm::reverse(CastExprStack)) {
+const CastExpr *Cast = &(CastRef.get());
+// Was there a previous cast?

nit, extra parens?


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 156983.
lebedev.ri added a comment.

Rebased on top of svn tip / git master, now that 
https://reviews.llvm.org/D49508 has landed,
which means there shouldn't be any more false-positives (and it's a bit faster 
to detect that the check shouldn't be emitted, too).

Ping, any more notes? :)


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,256 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned short)c;
+}
+
+// Implicit 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 156583.
lebedev.ri marked 11 inline comments as done.
lebedev.ri added a comment.

Address @vsk's review notes.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,256 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned short)c;
+}
+
+// Implicit truncation before explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference1
+unsigned char explicit_cast_interference1(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/CodeGen/CodeGenFunction.h:383
+  // This stack is used/maintained exclusively by the implicit cast sanitizer.
+  llvm::SmallVector CastExprStack;
+

vsk wrote:
> lebedev.ri wrote:
> > vsk wrote:
> > > lebedev.ri wrote:
> > > > vsk wrote:
> > > > > Why not 0 instead of 8, given that in the common case, this stack is 
> > > > > unused?
> > > > No longer relevant.
> > > I'm referring to CastExprStack within ScalarExprEmitter, which still 
> > > allocates space for 8 pointers inline.
> > Ah, you mean in the general case when the sanitizer is disabled?
> > 
> Yes. It's a relatively minor concern, but clang's stack can get pretty deep 
> inside of CodeGenFunction. At one point we needed to outline code by hand to 
> unbreak the ASan build. Later I think we just increased the stack size 
> rlimit. I don't see a countervailing performance benefit of allocating more 
> space inline, at least not here.
No, i agree and totally understand.
I just didn't think about that sanitizer-less context.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: lib/CodeGen/CodeGenFunction.h:383
+  // This stack is used/maintained exclusively by the implicit cast sanitizer.
+  llvm::SmallVector CastExprStack;
+

lebedev.ri wrote:
> vsk wrote:
> > lebedev.ri wrote:
> > > vsk wrote:
> > > > Why not 0 instead of 8, given that in the common case, this stack is 
> > > > unused?
> > > No longer relevant.
> > I'm referring to CastExprStack within ScalarExprEmitter, which still 
> > allocates space for 8 pointers inline.
> Ah, you mean in the general case when the sanitizer is disabled?
> 
Yes. It's a relatively minor concern, but clang's stack can get pretty deep 
inside of CodeGenFunction. At one point we needed to outline code by hand to 
unbreak the ASan build. Later I think we just increased the stack size rlimit. 
I don't see a countervailing performance benefit of allocating more space 
inline, at least not here.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/CodeGen/CodeGenFunction.h:383
+  // This stack is used/maintained exclusively by the implicit cast sanitizer.
+  llvm::SmallVector CastExprStack;
+

vsk wrote:
> lebedev.ri wrote:
> > vsk wrote:
> > > Why not 0 instead of 8, given that in the common case, this stack is 
> > > unused?
> > No longer relevant.
> I'm referring to CastExprStack within ScalarExprEmitter, which still 
> allocates space for 8 pointers inline.
Ah, you mean in the general case when the sanitizer is disabled?



Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: docs/UndefinedBehaviorSanitizer.rst:159
   -  ``-fsanitize=undefined``: All of the checks listed above other than
  ``unsigned-integer-overflow`` and the ``nullability-*`` checks.
   -  ``-fsanitize=undefined-trap``: Deprecated alias of

Please add "the `implicit-cast` group of checks" to this list.



Comment at: docs/UndefinedBehaviorSanitizer.rst:134
+ integer promotions, as those may result in an unexpected computation
+ results, even though no overflow happens (signed or unsigned).
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable

lebedev.ri wrote:
> vsk wrote:
> > Could you make this more explicit? It would help to point out that this 
> > check does not diagnose lossy implicit integer conversions, but that the 
> > new check does. Ditto for the comment in the unsigned-integer-overflow 
> > section.
> Is this better?
Looks good.



Comment at: lib/CodeGen/CodeGenFunction.h:383
+  // This stack is used/maintained exclusively by the implicit cast sanitizer.
+  llvm::SmallVector CastExprStack;
+

lebedev.ri wrote:
> vsk wrote:
> > Why not 0 instead of 8, given that in the common case, this stack is unused?
> No longer relevant.
I'm referring to CastExprStack within ScalarExprEmitter, which still allocates 
space for 8 pointers inline.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 156470.
lebedev.ri marked 6 inline comments as done.
lebedev.ri added a comment.

Rebased ontop of yet-again rewritten https://reviews.llvm.org/D49508.
Addressed all @vsk's review notes.

More review notes wanted :)


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,256 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned short)c;
+}
+
+// Implicit truncation before explicit truncation.
+// CHECK-LABEL: 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: docs/UndefinedBehaviorSanitizer.rst:134
+ integer promotions, as those may result in an unexpected computation
+ results, even though no overflow happens (signed or unsigned).
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable

vsk wrote:
> Could you make this more explicit? It would help to point out that this check 
> does not diagnose lossy implicit integer conversions, but that the new check 
> does. Ditto for the comment in the unsigned-integer-overflow section.
Is this better?



Comment at: lib/CodeGen/CodeGenFunction.h:383
+  // This stack is used/maintained exclusively by the implicit cast sanitizer.
+  llvm::SmallVector CastExprStack;
+

vsk wrote:
> Why not 0 instead of 8, given that in the common case, this stack is unused?
No longer relevant.



Comment at: lib/CodeGen/CodeGenFunction.h:388
+
+llvm::SmallVector GuardedCasts;
+

vsk wrote:
> I'm not sure the cost of maintaining an extra vector is worth the benefit of 
> the added assertion. Wouldn't it be cheaper to just store the number of 
> pushed casts? You'd only need one constructor which accepts an ArrayRef CastExpr *>.
No longer relevant.



Comment at: test/CodeGen/catch-implicit-integer-truncations.c:29
+  // CHECK-SANITIZE-NEXT: %[[TRUNCHECK:.*]] = icmp eq i32 %[[ANYEXT]], 
%[[SRC]], !nosanitize
+  // CHECK-SANITIZE-ANYRECOVER-NEXT: br i1 %[[TRUNCHECK]], label %[[CONT:.*]], 
label %[[HANDLER_IMPLICIT_CAST:.*]], !prof ![[WEIGHT_MD:.*]], !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT: br i1 %[[TRUNCHECK]], label %[[CONT:.*]], label 
%[[HANDLER_IMPLICIT_CAST:.*]], !nosanitize

vsk wrote:
> There's no need to check the profile metadata here.
I was checking it because otherwise `HANDLER_IMPLICIT_CAST` would have 
over-eagerly consumed `, !prof !3` too.
But there is actually a way around that..



Comment at: test/CodeGen/catch-implicit-integer-truncations.c:159
+// == 
//
+// The expected false-negatives.
+// == 
//

vsk wrote:
> nit, aren't these true-negatives because we expect to see no errors?
Right.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: docs/UndefinedBehaviorSanitizer.rst:97
+ is not equal to the original value before the downcast. This kind of 
issues
+ may often be caused by an implicit integer promotions.
   -  ``-fsanitize=integer-divide-by-zero``: Integer division by zero.

Nitpicks:
kind of issues -> issue
promotions -> conversions



Comment at: docs/UndefinedBehaviorSanitizer.rst:134
+ integer promotions, as those may result in an unexpected computation
+ results, even though no overflow happens (signed or unsigned).
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable

Could you make this more explicit? It would help to point out that this check 
does not diagnose lossy implicit integer conversions, but that the new check 
does. Ditto for the comment in the unsigned-integer-overflow section.



Comment at: lib/CodeGen/CodeGenFunction.h:383
+  // This stack is used/maintained exclusively by the implicit cast sanitizer.
+  llvm::SmallVector CastExprStack;
+

Why not 0 instead of 8, given that in the common case, this stack is unused?



Comment at: lib/CodeGen/CodeGenFunction.h:388
+
+llvm::SmallVector GuardedCasts;
+

I'm not sure the cost of maintaining an extra vector is worth the benefit of 
the added assertion. Wouldn't it be cheaper to just store the number of pushed 
casts? You'd only need one constructor which accepts an ArrayRef.



Comment at: test/CodeGen/catch-implicit-integer-truncations.c:29
+  // CHECK-SANITIZE-NEXT: %[[TRUNCHECK:.*]] = icmp eq i32 %[[ANYEXT]], 
%[[SRC]], !nosanitize
+  // CHECK-SANITIZE-ANYRECOVER-NEXT: br i1 %[[TRUNCHECK]], label %[[CONT:.*]], 
label %[[HANDLER_IMPLICIT_CAST:.*]], !prof ![[WEIGHT_MD:.*]], !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT: br i1 %[[TRUNCHECK]], label %[[CONT:.*]], label 
%[[HANDLER_IMPLICIT_CAST:.*]], !nosanitize

There's no need to check the profile metadata here.



Comment at: test/CodeGen/catch-implicit-integer-truncations.c:159
+// == 
//
+// The expected false-negatives.
+// == 
//

nit, aren't these true-negatives because we expect to see no errors?


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 156237.
lebedev.ri added a comment.

Breakthrough: no more false-positives due to the `MaterializeTemporaryExpr` 
skipping over NoOp casts. (https://reviews.llvm.org/D49508)
Slight docs update.

Ping, please review!
We are so close :)


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprConstant.cpp
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,254 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned short)c;
+}
+
+// Implicit 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Well, that's just great, with `isCastPartOfExplictCast()`, the 
`ASTContext::getParents()`
also does not return `CXXStaticCastExpr` as parent for such cases.
I don't know how to proceed.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 155363.
lebedev.ri marked 3 inline comments as done.
lebedev.ri added a comment.

Address @vsk review notes, although this will be revered by the next update 
dropping the faulty 'stack' optimization.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,255 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned short)c;
+}
+
+// Implicit truncation before explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference1
+unsigned 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 5 inline comments as done.
lebedev.ri added a comment.

@vsk so yeah, no wonder that doesn't work.
Somehow in that test case `ScalarExprEmitter::VisitExplicitCastExpr()` 
**never** gets called.
(I'm pretty sure this worked with the naive implementation, so worst case i'll 
just revert the 'stack' code)
Trying to assess the issue..




Comment at: lib/CodeGen/CGExprScalar.cpp:351
+ScalarConversionOpts()
+: TreatBooleanAsSigned(false),
+  EmitImplicitIntegerTruncationChecks(false) {}

lebedev.ri wrote:
> vsk wrote:
> > Why not use default member initializers here (e.g, "bool a = false")?
> I'll double-check, but i'm pretty sure then there were some warnings when i 
> did that,
> Or, the default needs to be defined in the actual declaration of 
> `EmitScalarConversion()`, i think.
```
[2/14 0.3/sec] Building CXX object 
tools/clang/lib/CodeGen/CMakeFiles/clangCodeGen.dir/CGExprScalar.cpp.o
FAILED: tools/clang/lib/CodeGen/CMakeFiles/clangCodeGen.dir/CGExprScalar.cpp.o 
/usr/bin/clang++-6.0  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE 
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-Itools/clang/lib/CodeGen -I/build/clang/lib/CodeGen -I/build/clang/include 
-Itools/clang/include -I/usr/include/libxml2 -Iinclude -I/build/llvm/include 
-g0 -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time 
-Werror=unguarded-availability-new -std=c++11 -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color 
-ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual 
-Wno-nested-anon-types -O3 -g0  -fPIC   -UNDEBUG  -fno-exceptions -fno-rtti -MD 
-MT tools/clang/lib/CodeGen/CMakeFiles/clangCodeGen.dir/CGExprScalar.cpp.o -MF 
tools/clang/lib/CodeGen/CMakeFiles/clangCodeGen.dir/CGExprScalar.cpp.o.d -o 
tools/clang/lib/CodeGen/CMakeFiles/clangCodeGen.dir/CGExprScalar.cpp.o -c 
/build/clang/lib/CodeGen/CGExprScalar.cpp
/build/clang/lib/CodeGen/CGExprScalar.cpp:355:52: error: default member 
initializer for 'TreatBooleanAsSigned' needed within definition of enclosing 
class 'ScalarExprEmitter' outside of member functions
   ScalarConversionOpts Opts = ScalarConversionOpts());
   ^
/build/clang/lib/CodeGen/CGExprScalar.cpp:349:10: note: default member 
initializer declared here
bool TreatBooleanAsSigned = false;
 ^
/build/clang/lib/CodeGen/CGExprScalar.cpp:355:52: error: default member 
initializer for 'EmitImplicitIntegerTruncationChecks' needed within definition 
of enclosing class 'ScalarExprEmitter' outside of member functions
   ScalarConversionOpts Opts = ScalarConversionOpts());
   ^
/build/clang/lib/CodeGen/CGExprScalar.cpp:350:10: note: default member 
initializer declared here
bool EmitImplicitIntegerTruncationChecks = false;
 ^
2 errors generated.
```



Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In https://reviews.llvm.org/D48958#1160494, @lebedev.ri wrote:

> In https://reviews.llvm.org/D48958#1160479, @vsk wrote:
>
> > In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote:
> >
> > > Thank you for taking a look!
> > >
> > > In https://reviews.llvm.org/D48958#1160381, @vsk wrote:
> > >
> > > > I have some minor comments but overall I think this is in good shape. 
> > > > It would be great to see some compile-time numbers just to make sure 
> > > > this is tractable. I'm pretty sure -fsanitize=null would fire more 
> > > > often across a codebase than this check, so I don't anticipate a big 
> > > > surprise here.
> > >
> > >
> > > Could you please clarify, which numbers are you looking for, specifically?
> > >  The time it takes to build llvm stage2 with `-fsanitize=implicit-cast`?
> > >  Or the time it takes to build llvm stage3 with compiler built with 
> > > `-fsanitize=implicit-cast`?
> >
> >
> > I had in mind measuring the difference between -fsanitize=undefined and 
> > -fsanitize=undefined,implicit-cast, with a stage2 compiler. I think that 
> > captures the expected use case: existing ubsan users enabling this new 
> > check.
>
>
> FWIW, i'm trying to look into optimizing these new IR patterns right now 
> https://reviews.llvm.org/D49179 https://reviews.llvm.org/D49247.
>
> >> (The numbers won't be too representable, whole stage-1 takes ~40 minutes 
> >> here...)
> > 
> > Ah I see, I'll run a few builds and take a stab at it, then.
>
> Yes, please, thank you!


The stage2 build traps before it finishes:

  FAILED: lib/IR/AttributesCompatFunc.inc.tmp
  cd /Users/vsk/src/builds/llvm.org-lldbsan-stage2-R/tools/clang/stage2-bins && 
/Users/vsk/src/builds/llvm.org-lldbsan-stage2-R/tools/clang/stage2-bins/bin/llvm-tblgen
 -gen-attrs -I /Users/vsk/src/llvm.org-lldbsan/llvm/lib/IR -I 
/Users/vsk/src/llvm.org-lldbsan/llvm/include 
/Users/vsk/src/llvm.org-lldbsan/llvm/lib/IR/AttributesCompatFunc.td -o 
lib/IR/AttributesCompatFunc.inc.tmp -d lib/IR/AttributesCompatFunc.inc.d
  /Users/vsk/src/llvm.org-lldbsan/llvm/include/llvm/ADT/DenseMap.h:732:66: 
runtime error: implicit cast from type 'uint64_t' (aka 'unsigned long long') of 
value 4294967296 (64-bit, unsigned) to type 'unsigned int' changed the value to 
0 (32-bit, unsigned)
  /bin/sh: line 1: 96848 Abort trap: 6

This looks like a false positive to me. It's complaining about 
`static_cast(NextPowerOf2(...))`, but the static_cast is explicit.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D48958#1160848, @vsk wrote:

> In https://reviews.llvm.org/D48958#1160494, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D48958#1160479, @vsk wrote:
> >
> > > In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote:
> > >
> > > > Thank you for taking a look!
> > > >
> > > > In https://reviews.llvm.org/D48958#1160381, @vsk wrote:
> > > >
> > > > > I have some minor comments but overall I think this is in good shape. 
> > > > > It would be great to see some compile-time numbers just to make sure 
> > > > > this is tractable. I'm pretty sure -fsanitize=null would fire more 
> > > > > often across a codebase than this check, so I don't anticipate a big 
> > > > > surprise here.
> > > >
> > > >
> > > > Could you please clarify, which numbers are you looking for, 
> > > > specifically?
> > > >  The time it takes to build llvm stage2 with `-fsanitize=implicit-cast`?
> > > >  Or the time it takes to build llvm stage3 with compiler built with 
> > > > `-fsanitize=implicit-cast`?
> > >
> > >
> > > I had in mind measuring the difference between -fsanitize=undefined and 
> > > -fsanitize=undefined,implicit-cast, with a stage2 compiler. I think that 
> > > captures the expected use case: existing ubsan users enabling this new 
> > > check.
> >
> >
> > FWIW, i'm trying to look into optimizing these new IR patterns right now 
> > https://reviews.llvm.org/D49179 https://reviews.llvm.org/D49247.
> >
> > >> (The numbers won't be too representable, whole stage-1 takes ~40 minutes 
> > >> here...)
> > > 
> > > Ah I see, I'll run a few builds and take a stab at it, then.
> >
> > Yes, please, thank you!
>
>
> The stage2 build traps before it finishes:
>
>   FAILED: lib/IR/AttributesCompatFunc.inc.tmp
>   cd /Users/vsk/src/builds/llvm.org-lldbsan-stage2-R/tools/clang/stage2-bins 
> && 
> /Users/vsk/src/builds/llvm.org-lldbsan-stage2-R/tools/clang/stage2-bins/bin/llvm-tblgen
>  -gen-attrs -I /Users/vsk/src/llvm.org-lldbsan/llvm/lib/IR -I 
> /Users/vsk/src/llvm.org-lldbsan/llvm/include 
> /Users/vsk/src/llvm.org-lldbsan/llvm/lib/IR/AttributesCompatFunc.td -o 
> lib/IR/AttributesCompatFunc.inc.tmp -d lib/IR/AttributesCompatFunc.inc.d
>   /Users/vsk/src/llvm.org-lldbsan/llvm/include/llvm/ADT/DenseMap.h:732:66: 
> runtime error: implicit cast from type 'uint64_t' (aka 'unsigned long long') 
> of value 4294967296 (64-bit, unsigned) to type 'unsigned int' changed the 
> value to 0 (32-bit, unsigned)
>   /bin/sh: line 1: 96848 Abort trap: 6
>
>
> This looks like a false positive to me. It's complaining about 
> `static_cast(NextPowerOf2(...))`, but the static_cast is explicit.


Good to know, so the stack-based logic for `ExplicitCastExpr` detection needs 
further tests/refinements..


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D48958#1160853, @lebedev.ri wrote:

> In https://reviews.llvm.org/D48958#1160848, @vsk wrote:
>
> > <...>
> >  The stage2 build traps before it finishes:
> >
> >   FAILED: lib/IR/AttributesCompatFunc.inc.tmp
> >   cd 
> > /Users/vsk/src/builds/llvm.org-lldbsan-stage2-R/tools/clang/stage2-bins && 
> > /Users/vsk/src/builds/llvm.org-lldbsan-stage2-R/tools/clang/stage2-bins/bin/llvm-tblgen
> >  -gen-attrs -I /Users/vsk/src/llvm.org-lldbsan/llvm/lib/IR -I 
> > /Users/vsk/src/llvm.org-lldbsan/llvm/include 
> > /Users/vsk/src/llvm.org-lldbsan/llvm/lib/IR/AttributesCompatFunc.td -o 
> > lib/IR/AttributesCompatFunc.inc.tmp -d lib/IR/AttributesCompatFunc.inc.d
> >   /Users/vsk/src/llvm.org-lldbsan/llvm/include/llvm/ADT/DenseMap.h:732:66: 
> > runtime error: implicit cast from type 'uint64_t' (aka 'unsigned long 
> > long') of value 4294967296 (64-bit, unsigned) to type 'unsigned int' 
> > changed the value to 0 (32-bit, unsigned)
> >   /bin/sh: line 1: 96848 Abort trap: 6
> >
> >
> > This looks like a false positive to me. It's complaining about 
> > `static_cast(NextPowerOf2(...))`, but the static_cast is explicit.
>
>
> Good to know, so the stack-based logic for `ExplicitCastExpr` detection needs 
> further tests/refinements..


creduced down to:

  template  a b(a c, const a ) {
if (d)
  ;
return c;
  }
  int e = b(4, static_cast(4294967296));
  int main() {}

https://godbolt.org/g/1kwGk9

  $ ./a.out 
  test.cpp:6:46: runtime error: implicit cast from type 'long' of value 
4294967296 (64-bit, signed) to type 'unsigned int' changed the value to 0 
(32-bit, unsigned)
  #0 0x232f56 in _GLOBAL__sub_I_test.cpp 
(/home/lebedevri/CREDUCE/a.out+0x232f56)
  #1 0x232fbc in __libc_csu_init (/home/lebedevri/CREDUCE/a.out+0x232fbc)
  #2 0x7fa8c113aaa7 in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x22aa7)
  #3 0x212029 in _start (/home/lebedevri/CREDUCE/a.out+0x212029)


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thank you for taking a look!

In https://reviews.llvm.org/D48958#1160381, @vsk wrote:

> I have some minor comments but overall I think this is in good shape. It 
> would be great to see some compile-time numbers just to make sure this is 
> tractable. I'm pretty sure -fsanitize=null would fire more often across a 
> codebase than this check, so I don't anticipate a big surprise here.


Could you please clarify, which numbers are you looking for, specifically?
The time it takes to build llvm stage2 with `-fsanitize=implicit-cast`?
Or the time it takes to build llvm stage3 with compiler built with 
`-fsanitize=implicit-cast`?
(The numbers won't be too representable, whole stage-1 takes ~40 minutes 
here...)




Comment at: lib/CodeGen/CGExprScalar.cpp:351
+ScalarConversionOpts()
+: TreatBooleanAsSigned(false),
+  EmitImplicitIntegerTruncationChecks(false) {}

vsk wrote:
> Why not use default member initializers here (e.g, "bool a = false")?
I'll double-check, but i'm pretty sure then there were some warnings when i did 
that,
Or, the default needs to be defined in the actual declaration of 
`EmitScalarConversion()`, i think.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote:

> Thank you for taking a look!
>
> In https://reviews.llvm.org/D48958#1160381, @vsk wrote:
>
> > I have some minor comments but overall I think this is in good shape. It 
> > would be great to see some compile-time numbers just to make sure this is 
> > tractable. I'm pretty sure -fsanitize=null would fire more often across a 
> > codebase than this check, so I don't anticipate a big surprise here.
>
>
> Could you please clarify, which numbers are you looking for, specifically?
>  The time it takes to build llvm stage2 with `-fsanitize=implicit-cast`?
>  Or the time it takes to build llvm stage3 with compiler built with 
> `-fsanitize=implicit-cast`?


I had in mind measuring the difference between -fsanitize=undefined and 
-fsanitize=undefined,implicit-cast, with a stage2 compiler. I think that 
captures the expected use case: existing ubsan users enabling this new check.

> (The numbers won't be too representable, whole stage-1 takes ~40 minutes 
> here...)

Ah I see, I'll run a few builds and take a stab at it, then.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

I have some minor comments but overall I think this is in good shape. It would 
be great to see some compile-time numbers just to make sure this is tractable. 
I'm pretty sure -fsanitize=null would fire more often across a codebase than 
this check, so I don't anticipate a big surprise here.




Comment at: lib/CodeGen/CGExprScalar.cpp:222
+
+  // The stack of currently-visiting Cast expressions.
+  llvm::SmallVector CastExprStack;

It would help to have this comment explain that the stack is used/maintained 
exclusively by the implicit cast sanitizer.



Comment at: lib/CodeGen/CGExprScalar.cpp:228
+
+// We only care if we are to use this data.
+bool Enabled() {

Could you make this comment more specific -- maybe by explaining that for 
efficiency reasons, the cast expr stack is only maintained when a sanitizer 
check is enabled?



Comment at: lib/CodeGen/CGExprScalar.cpp:234
+  public:
+CastExprStackGuard(ScalarExprEmitter *SEE, CastExpr *CE) : SEE(SEE) {
+  if (!Enabled())

I think if you were to use references instead of pointers here, the code would 
be a bit clearer, and you wouldn't need to assert that CE is non-null.



Comment at: lib/CodeGen/CGExprScalar.cpp:351
+ScalarConversionOpts()
+: TreatBooleanAsSigned(false),
+  EmitImplicitIntegerTruncationChecks(false) {}

Why not use default member initializers here (e.g, "bool a = false")?



Comment at: lib/CodeGen/CGExprScalar.cpp:988
+  return Child == PreviousCast;
+}))
+  return false;

The none_of call could safely be replaced by `Cast->getSubExpr() != 
PreviousCast`, I think.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D48958#1160479, @vsk wrote:

> In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote:
>
> > Thank you for taking a look!
> >
> > In https://reviews.llvm.org/D48958#1160381, @vsk wrote:
> >
> > > I have some minor comments but overall I think this is in good shape. It 
> > > would be great to see some compile-time numbers just to make sure this is 
> > > tractable. I'm pretty sure -fsanitize=null would fire more often across a 
> > > codebase than this check, so I don't anticipate a big surprise here.
> >
> >
> > Could you please clarify, which numbers are you looking for, specifically?
> >  The time it takes to build llvm stage2 with `-fsanitize=implicit-cast`?
> >  Or the time it takes to build llvm stage3 with compiler built with 
> > `-fsanitize=implicit-cast`?
>
>
> I had in mind measuring the difference between -fsanitize=undefined and 
> -fsanitize=undefined,implicit-cast, with a stage2 compiler. I think that 
> captures the expected use case: existing ubsan users enabling this new check.


FWIW, i'm trying to look into optimizing these new IR patterns right now 
https://reviews.llvm.org/D49179 https://reviews.llvm.org/D49247.

>> (The numbers won't be too representable, whole stage-1 takes ~40 minutes 
>> here...)
> 
> Ah I see, I'll run a few builds and take a stab at it, then.

Yes, please, thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:1694
 // handle things like function to ptr-to-function decay etc.
 Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
   Expr *E = CE->getSubExpr();

vsk wrote:
> lebedev.ri wrote:
> > vsk wrote:
> > > lebedev.ri wrote:
> > > > vsk wrote:
> > > > > I think maintaining a stack of visited cast exprs in the emitter be 
> > > > > cheaper/simpler than using ASTContext::getParents. You could push CE 
> > > > > here and use a RAII helper to pop it. The 'isCastPartOfExplicitCast' 
> > > > > check then simplifies to a quick stack traversal.
> > > > Hmm, two things come to mind:
> > > > 1. This pessimizes the (most popular) case when the sanitizer is 
> > > > disabled.
> > > > 2. `ASTContext::getParents()` may return more than one parent. I'm not 
> > > > sure if that matters here?
> > > > I'll take a look..
> > > As for (1), it's not necessary to push/pop the stack when this sanitizer 
> > > is disabled. And for (2), IIUC the only interesting case is 
> > > "explicit-cast +", and none of the implicit casts here 
> > > have more than one parent.
> > > I think maintaining a stack of visited cast exprs in the emitter be 
> > > cheaper/simpler than using ASTContext::getParents. 
> > 
> > {F6660377}
> > 
> > So yeah, this could work.
> > Except sadly it breaks in subtle cases like https://godbolt.org/g/5V2czU
> > I have added those tests beforehand.
> > 
> > Is `ASTContext::getParents()` really horribly slow so we want to 
> > duplicate/maintain/track
> > the current AST stack ourselves?
> > If so, we will need to maintain the *entire* stack, not just `CastExpr''s...
> I think the scan in 'IsTopCastPartOfExplicitCast' can be fixed: while 
> traversing backwards, you'd need to check that the previously-visited cast 
> expr is the child of the current expr. That should address the false negative 
> you pointed out in interference1. I don't yet see what the issue is with 
> interference0.
> 
> Could you explain why maintaining a stack of unfinished casts wouldn't work? 
> I don't understand why you'd need the entire stack. My sense is that it's not 
> required to match the "explicit-cast +" pattern, but I could 
> easily be missing something here. As for why this might be worth looking 
> into, I think scanning for an explicit cast is much easier to understand when 
> working with a stack.
> 
> + @klimek to comment on what to expect in terms of the overhead of 
> ASTContext::getParents. Regardless of what approach we pick, it would help to 
> see pre/post-patch compile times for a stage2 build of something like clang 
> or llc.
> I think the scan in 'IsTopCastPartOfExplicitCast' can be fixed: while 
> traversing backwards, you'd need to check that the previously-visited cast 
> expr is the child of the current expr. That should address the false negative 
> you pointed out in interference1.
Oh right. That seems to fix the issues.

> Could you explain why maintaining a stack of unfinished casts wouldn't work?
I didn't think it wouldn't work. I just missed the tidbit about checking 
children.
Then it works.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 155149.
lebedev.ri marked 7 inline comments as done.
lebedev.ri added a comment.

Address @vsk's review notes.

- Maintain the stack of currently-being-visited `CastExpr`'s
- Use that stack to check whether we are in a `ExplicitCastExpr`
- Move logic for deciding whether to emit the check out of 
`EmitScalarConversion()`
- Condense all overloads of `EmitScalarConversion()` down to one.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,230 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a subscriber: klimek.
vsk added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:1694
 // handle things like function to ptr-to-function decay etc.
 Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
   Expr *E = CE->getSubExpr();

lebedev.ri wrote:
> vsk wrote:
> > lebedev.ri wrote:
> > > vsk wrote:
> > > > I think maintaining a stack of visited cast exprs in the emitter be 
> > > > cheaper/simpler than using ASTContext::getParents. You could push CE 
> > > > here and use a RAII helper to pop it. The 'isCastPartOfExplicitCast' 
> > > > check then simplifies to a quick stack traversal.
> > > Hmm, two things come to mind:
> > > 1. This pessimizes the (most popular) case when the sanitizer is disabled.
> > > 2. `ASTContext::getParents()` may return more than one parent. I'm not 
> > > sure if that matters here?
> > > I'll take a look..
> > As for (1), it's not necessary to push/pop the stack when this sanitizer is 
> > disabled. And for (2), IIUC the only interesting case is "explicit-cast 
> > +", and none of the implicit casts here have more than one 
> > parent.
> > I think maintaining a stack of visited cast exprs in the emitter be 
> > cheaper/simpler than using ASTContext::getParents. 
> 
> {F6660377}
> 
> So yeah, this could work.
> Except sadly it breaks in subtle cases like https://godbolt.org/g/5V2czU
> I have added those tests beforehand.
> 
> Is `ASTContext::getParents()` really horribly slow so we want to 
> duplicate/maintain/track
> the current AST stack ourselves?
> If so, we will need to maintain the *entire* stack, not just `CastExpr''s...
I think the scan in 'IsTopCastPartOfExplicitCast' can be fixed: while 
traversing backwards, you'd need to check that the previously-visited cast expr 
is the child of the current expr. That should address the false negative you 
pointed out in interference1. I don't yet see what the issue is with 
interference0.

Could you explain why maintaining a stack of unfinished casts wouldn't work? I 
don't understand why you'd need the entire stack. My sense is that it's not 
required to match the "explicit-cast +" pattern, but I could 
easily be missing something here. As for why this might be worth looking into, 
I think scanning for an explicit cast is much easier to understand when working 
with a stack.

+ @klimek to comment on what to expect in terms of the overhead of 
ASTContext::getParents. Regardless of what approach we pick, it would help to 
see pre/post-patch compile times for a stage2 build of something like clang or 
llc.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 155064.
lebedev.ri added a comment.

Add some more tricky tests where maintaining just the `CastExpr` part of AST 
stack would break them.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,230 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned short)c;
+}
+
+// Implicit truncation before explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference1
+unsigned char explicit_cast_interference1(unsigned int c) {
+  // 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:1694
 // handle things like function to ptr-to-function decay etc.
 Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
   Expr *E = CE->getSubExpr();

vsk wrote:
> lebedev.ri wrote:
> > vsk wrote:
> > > I think maintaining a stack of visited cast exprs in the emitter be 
> > > cheaper/simpler than using ASTContext::getParents. You could push CE here 
> > > and use a RAII helper to pop it. The 'isCastPartOfExplicitCast' check 
> > > then simplifies to a quick stack traversal.
> > Hmm, two things come to mind:
> > 1. This pessimizes the (most popular) case when the sanitizer is disabled.
> > 2. `ASTContext::getParents()` may return more than one parent. I'm not sure 
> > if that matters here?
> > I'll take a look..
> As for (1), it's not necessary to push/pop the stack when this sanitizer is 
> disabled. And for (2), IIUC the only interesting case is "explicit-cast 
> +", and none of the implicit casts here have more than one 
> parent.
> I think maintaining a stack of visited cast exprs in the emitter be 
> cheaper/simpler than using ASTContext::getParents. 

{F6660377}

So yeah, this could work.
Except sadly it breaks in subtle cases like https://godbolt.org/g/5V2czU
I have added those tests beforehand.

Is `ASTContext::getParents()` really horribly slow so we want to 
duplicate/maintain/track
the current AST stack ourselves?
If so, we will need to maintain the *entire* stack, not just `CastExpr''s...


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:1694
 // handle things like function to ptr-to-function decay etc.
 Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
   Expr *E = CE->getSubExpr();

lebedev.ri wrote:
> vsk wrote:
> > I think maintaining a stack of visited cast exprs in the emitter be 
> > cheaper/simpler than using ASTContext::getParents. You could push CE here 
> > and use a RAII helper to pop it. The 'isCastPartOfExplicitCast' check then 
> > simplifies to a quick stack traversal.
> Hmm, two things come to mind:
> 1. This pessimizes the (most popular) case when the sanitizer is disabled.
> 2. `ASTContext::getParents()` may return more than one parent. I'm not sure 
> if that matters here?
> I'll take a look..
As for (1), it's not necessary to push/pop the stack when this sanitizer is 
disabled. And for (2), IIUC the only interesting case is "explicit-cast 
+", and none of the implicit casts here have more than one 
parent.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:318
 
-  Value *EmitScalarConversion(Value *Src, QualType SrcTy, QualType DstTy,
+  Value *EmitScalarConversion(Value *Src, QualType SrcType, QualType DstType,
   SourceLocation Loc, bool TreatBooleanAsSigned);

vsk wrote:
> I think the number of overloads here is really unwieldy. There should be a 
> simpler way to structure this. What about consolidating all four overloads 
> into one? Maybe:
> 
> ```
> struct ScalarConversionsOpts {
>   bool TreatBoolAsUnsigned = false;
>   bool EmitImplicitIntegerTruncationCheck = false;
> };
> 
> Value *EmitScalarConversion(Src, SrcTy, DstTy, Loc, Opts = 
> ScalarConversionOpts())
> ```
> 
> It's not necessary to pass CastExpr in, right? There's only one place where 
> that's done. It seems simpler to just do the SanOpts / 
> isCastPartOfExplicitCast checking there.
The number of overloads is indeed unwieldy.



Comment at: lib/CodeGen/CGExprScalar.cpp:1694
 // handle things like function to ptr-to-function decay etc.
 Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
   Expr *E = CE->getSubExpr();

vsk wrote:
> I think maintaining a stack of visited cast exprs in the emitter be 
> cheaper/simpler than using ASTContext::getParents. You could push CE here and 
> use a RAII helper to pop it. The 'isCastPartOfExplicitCast' check then 
> simplifies to a quick stack traversal.
Hmm, two things come to mind:
1. This pessimizes the (most popular) case when the sanitizer is disabled.
2. `ASTContext::getParents()` may return more than one parent. I'm not sure if 
that matters here?
I'll take a look..


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Thanks for working on this!




Comment at: docs/ReleaseNotes.rst:277
+  behaviour. But they are not *always* intentional, and are somewhat hard to
+  track down.
 

Could you mention whether the group is enabled by -fsanitize=undefined?



Comment at: lib/CodeGen/CGExprScalar.cpp:318
 
-  Value *EmitScalarConversion(Value *Src, QualType SrcTy, QualType DstTy,
+  Value *EmitScalarConversion(Value *Src, QualType SrcType, QualType DstType,
   SourceLocation Loc, bool TreatBooleanAsSigned);

I think the number of overloads here is really unwieldy. There should be a 
simpler way to structure this. What about consolidating all four overloads into 
one? Maybe:

```
struct ScalarConversionsOpts {
  bool TreatBoolAsUnsigned = false;
  bool EmitImplicitIntegerTruncationCheck = false;
};

Value *EmitScalarConversion(Src, SrcTy, DstTy, Loc, Opts = 
ScalarConversionOpts())
```

It's not necessary to pass CastExpr in, right? There's only one place where 
that's done. It seems simpler to just do the SanOpts / isCastPartOfExplicitCast 
checking there.



Comment at: lib/CodeGen/CGExprScalar.cpp:951
+//   implicit cast -> explicit cast <- that is an explicit 
cast.
+static bool CastIsPartOfExplictCast(ASTContext , const Expr *E) {
+  // If it's a nulllptr, or not a cast, then it's not a part of Explict Cast.

nit, function names typically begin with a verb: 'isCastPartOf...'



Comment at: lib/CodeGen/CGExprScalar.cpp:1694
 // handle things like function to ptr-to-function decay etc.
 Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
   Expr *E = CE->getSubExpr();

I think maintaining a stack of visited cast exprs in the emitter be 
cheaper/simpler than using ASTContext::getParents. You could push CE here and 
use a RAII helper to pop it. The 'isCastPartOfExplicitCast' check then 
simplifies to a quick stack traversal.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 154877.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.

@vsk thank you for taking a look!

Addressed the trivial part of nits.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,205 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// For now, all the true-positives are tested in the C test in ../CodeGen/
+
+// == //
+// The expected false-negatives.
+// == //
+
+// Explicit truncating casts.
+// == //
+
+// CHECK-LABEL: @explicit_unsigned_int_to_unsigned_char
+unsigned char explicit_unsigned_int_to_unsigned_char(unsigned int src) {
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned char)src;
+}
+
+// CHECK-LABEL: @explicit_signed_int_to_unsigned_char
+unsigned char explicit_signed_int_to_unsigned_char(signed int src) {
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned char)src;
+}
+
+// CHECK-LABEL: @explicit_unsigned_int_to_signed_char
+signed char 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 154863.
lebedev.ri added a comment.

- Check that sanitizer is actually enabled before doing the AST upwalk. I 
didn't measure, but it would be logical for this to be better.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,205 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// For now, all the true-positives are tested in the C test in ../CodeGen/
+
+// == //
+// The expected false-negatives.
+// == //
+
+// Explicit truncating casts.
+// == //
+
+// CHECK-LABEL: @explicit_unsigned_int_to_unsigned_char
+unsigned char explicit_unsigned_int_to_unsigned_char(unsigned int src) {
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned char)src;
+}
+
+// CHECK-LABEL: @explicit_signed_int_to_unsigned_char
+unsigned char explicit_signed_int_to_unsigned_char(signed int src) {
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned char)src;
+}
+
+// CHECK-LABEL: 

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Finished running it on a normal testset of my pet project 
.

- It fired ~18 times.
- There were no obvious false-positives (e.g. when an explicit cast was 
involved).
- At least 3 of those appear to be a true bugs.
- 4-5 more are probably bugs, but it is hard to tell.
- Last 10-11 appear to be mostly OK intentional truncating casts.

This was on a normal test set, i suspect fuzzing will reveal more.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 154591.
lebedev.ri retitled this revision from "[private][clang] Implicit Cast 
Sanitizer - integer truncation  - clang part" to "[clang][ubsan] Implicit Cast 
Sanitizer - integer truncation  - clang part".
lebedev.ri edited the summary of this revision.
lebedev.ri added reviewers: rjmccall, rsmith, samsonov, pcc, vsk, eugenis, 
efriedma, kcc.
lebedev.ri added a project: Sanitizers.
lebedev.ri added subscribers: cfe-commits, mclow.lists, milianw, dvyukov, 
ygribov, danielaustin, filcab, dtzWill, RKSimon, aaron.ballman, Sanitizers.
lebedev.ri added a comment.

C and C++ are interesting languages. They are statically typed, but weakly.
The implicit conversions are allowed. This is nice, allows to write code
while balancing between getting drowned in everything being convertible,
and nothing being convertible. As usual, this comes with a price:

  unsigned char store = 0;
  
  bool consume(unsigned int val);
  
  void test(unsigned long val) {
if (consume(val)) {
  // the 'val' is `unsigned long`, but `consume()` takes `unsigned int`.
  // If their bit widths are different on this platform, the implicit
  // truncation happens. And if that `unsigned long` had a value bigger
  // than UINT_MAX, then you may or may not have a bug.
  
  // Similarly, integer addition happens on `int`s, so `store` will
  // be promoted to an `int`, the sum calculated (0+768=768),
  // and the result demoted to `unsigned char`, and stored to `store`.
  // In this case, the `store` will still be 0. Again, not always intended.
  store = store + 768; // before addition, 'store' was promoted to int.
}
  
// But yes, sometimes this is intentional.
// You can either make the cast explicit
(void)consume((unsigned int)val);
// or mask the value so no bits will be *implicitly* lost.
(void)consume((~((unsigned int)0)) & val);
  }

Yes, there is a `-Wconversion`` diagnostic group, but first, it is kinda
noisy, since it warns on everything (unlike sanitizers, warning on an
actual issues), and second, there are cases where it does **not** warn.
So a Sanitizer is needed. I don't have any motivational numbers, but i know
i had this kind of problem 10-20 times, and it was never easy to track down.

The logic to detect whether an truncation has happened is pretty simple
if you think about it - https://godbolt.org/g/NEzXbb - basically, just
extend (using the new, not original!, signedness) the 'truncated' value
back to it's original width, and equality-compare it with the original value.

The most non-trivial thing here is the logic to detect whether this
`ImplicitCastExpr` AST node is **actually** an implicit cast, //or// part of
an explicit cast. Because the explicit casts are modeled as an outer
`ExplicitCastExpr` with some `ImplicitCastExpr`'s as **direct** children.
https://godbolt.org/g/eE1GkJ

It would //seem//, we can just check that the current `ImplicitCastExpr`
we are processing either has no `CastExpr` parents, or all of them are
`ImplicitCastExpr`.

As you may have noted, this isn't just named 
`-fsanitize=implicit-integer-truncation`.
There are potentially some more implicit casts to be warned about.
Namely, implicit casts that result in sign change; implicit cast
between different floating point types, or between fp and an integer,
when again, that conversion is lossy.

I suspect, there may be some false-negatives, cases yet to be handled.

This is a clang part.
The compiler-rt part is https://reviews.llvm.org/D48959.

Fixes PR21530 , PR37552 
, PR35409 
.
Partially fixes PR9821 .
Fixes https://github.com/google/sanitizers/issues/940.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target