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

[Chris Lattner]

On Sep 2, 2010, at 3:19 PM, John Regehr wrote:

> Another draft of this patch is attached!
> 
> John<clang-standalone-112808.patch>_______________________________________________

Great!  Thanks for proposing this.  Here are some thoughts:

+static unsigned checkValueType(QualType opType, const llvm::Value *value) {
+  if(value == 0)
+    return 6;
+  else {
+    if(opType->isIntegerType()) {
+      if(opType->isSignedIntegerType()) {
+        if(value->getType()->isIntegerTy(32))
+	  return 2;
+        else if(value->getType()->isIntegerTy(64))
+	  return 3;
+        else 
+          return 6;
+      }
+      else {
+        if(value->getType()->isIntegerTy(32))
+	  return 0;
+        else if(value->getType()->isIntegerTy(64))
+	  return 1;
+        else
+          return 6;
+      }

Please follow the llvm conventions.  The indentation is consistent here, please put a space after if (and while and other control flow statements), and there is no need for an "else" after a returns (reducing indentation is good).  These sorts of minor syntactic things occur throughout the patch.

Please also add doxygen comments for functions like this, it isn't clear what it is doing.

In terms of the actual shift check, there is no need to check for "x < 0", an unsigned comparison against the max value will encompass this.


In terms of the feature, I don't think that -fshl-checks-extension is a great name for the command line argument.  If this were just about checking for undefined behavior around shift left, then this should be part of -ftrapv.

I'm not sure exactly what the code is doing yet, but it looks like the interesting thing here is that it provides the capability to *report* where a problem occurs, instead of just terminating the app as with -ftrapv.  If this is the essential new thing, then it seems better to add a new level to the "default,-fwrapv,-ftrapv" hierarchy, maybe a new -fcheckv option.  This option would do the same set of checks as -ftrapv, but would report (in a nice way with file/line/col + string info) the error before dying.

I think the right steps to get this work into the tree are:

1) Add the new checking for shift under the existing -ftrapv umbrella, and make it work with that system.
2) add a new -fcheckv option to the driver, make it start out as a synonym for -ftrapv, but do all the plumbing.
3) enhance -fcheckv to produce the useful information on failure, migrating one -ftrapv check at a time to take advantage of it.

Does this seem like a reasonable path forward, or did I miss the mark on what this is trying to do?

-Chris