[cfe-dev] request for comments on patch: detecting integer undefined behaviors

[Eli Friedman]

On Sun, Aug 29, 2010 at 12:37 PM, John Regehr <regehr at cs.utah.edu> wrote:
>> 6. Please get rid of UndefinedBehaviorAddCheck,
>> UndefinedBehaviorSubCheck, and UndefinedBehaviorMulCheck; if there are
>> issues, we'd prefer to fix them in the LLVM backend (I'll try to fix
>> PR7838 sometime in the near future).
>
> This is done.  Our patched clang now crashes when compiling signed 64-bit
> multiplies on a 32-bit architecture, until that PR is fixed.
>
> Also you asked for a single flag that requests undefined behavior checking,
> and then it keys off of the language that was chosen.  This turns out to be
> a bad idea because Clang defaults to c99 mode, causing the aggressive shift
> checks to be added.  Please just trust us that very few people want to see
> reports that for example 1<<31 is undefined!  All real C codes contain lots
> of these "problems".  In the attached patch, these checks are turned off by
> default for all languages, but can be enabled with -fshl-checks-extension
> for people who do want to try to be clean with respect to the c99/c++0x
> standards.

Okay, that's fine.

A few more review comments:
+    if(CGF.rvOpTy->getScalarSizeInBits() == 32) {
+      IntMin = llvm::ConstantInt::get(VMContext, CGF.APInt32Min);
+      llvm::APInt APIntNegOne = llvm::APInt(32, -1, true);
+      NegOne = llvm::ConstantInt::get(VMContext, APIntNegOne);
+    }
+    else if(CGF.rvOpTy->getScalarSizeInBits() == 64) {
+      IntMin = llvm::ConstantInt::get(VMContext, CGF.APInt64Min);
+      llvm::APInt APIntNegOne = llvm::APInt(64, -1, true);
+      NegOne = llvm::ConstantInt::get(VMContext, APIntNegOne);
+    }

The if/else if shouldn't be necessary.

+      if(Ops.Ty->isFloatingType()) {
+        CGF.CreateTrapBB();
+        tmpRule = "Floating Division: Divisor is 0";
+        CGF.ATEI.setAll(Ops.LHS, Ops.RHS, Ops.E, DivFinalEnd, tmpRule);
+        CGF.Builder.CreateCondBr(Builder.CreateFCmpOEQ(Ops.RHS,
Zero), CGF.getSoleTrapBB(), DivFinalEnd);
+        CGF.EmitTrapBB();
+      }
+      else if(Ops.Ty->isRealFloatingType()) {
+        CGF.CreateTrapBB();
+        tmpRule = "Real Floating Division: Divisor is 0";
+        CGF.ATEI.setAll(Ops.LHS, Ops.RHS, Ops.E, DivFinalEnd, tmpRule);
+        CGF.Builder.CreateCondBr(Builder.CreateFCmpUEQ(Ops.RHS,
Zero), CGF.getSoleTrapBB(), DivFinalEnd);
+        CGF.EmitTrapBB();
+      }

The code in the else-if never runs: isFloatingType is a superset of
isRealFloatingType.

+  llvm::Value* trapHandlerResult;

Write-only variable?

+  // Converted operator's llvm::type
+  const llvm::Type* rvOpTy;

Is this really necessary?  Either way, I have no idea what the name is
supposed to mean, and the comment should make it clear this is used
for -fcatch-undefined-behavior machinery.

Also, the indentation looks like it's off in a few places, and some
lines go over 80 columns.

-Eli