[PATCH] D10634: Loss of Sign Checker

[Soumitra Chatterjee]

soumitra added a comment.

In http://reviews.llvm.org/D10634#213835, @zaks.anna wrote:

> Why do you have checkASTDecl (which is a syntactic check) in addition to the checkBind (which is a path-sensitive check)? Does checkASTDecl find more issues? can those be found using a path sensitive callback?


Yes, it seems that the global assignments can only be caught using the ASTDecl checker (probably because they do not have an associated "path"?) Please do let me know in case I am wrong in my understanding.

> I am leaning toward allowing explicit assignments to "-1", like in this case: "unsigned int j = -1". The tool is much more usable if there are few false positives.


This is exactly what I started off with, albeit with a plain 'char' instead of 'unsigned int'. We were hitting a runtime issue while porting a large piece of software to AArch64 since the "signedness" of plain char changes across x86 and AArch64, and a negative value was used as a initializer.

> Some of the results look like false positives. For example, this one from https://drive.google.com/file/d/0BykPmWrCOxt2aGtRNTY4eXQ1OU0/view: 

>  fitscore.c:1077:21: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

> 

>   namelen -= 9;  /* deleted the string 'HIERARCH ' */


I don't see the above in the link you posted. Can you please point me to the source so that I can look into this?
>From similar cases that Daniel pointed out earlier, it seems that the variable would possibly take a negative value (e.g. if namelen was initialized to 0) and hence the checker flags it off.

> It's hard to know what they are if we are only looking at the line where the issue is reported without seeing the pull path. Do we know that the value can be negative on that path?


What I am trying to do is check if the RHS of an unsigned initialization is negative and if so, flag off that assignment. 
Do you see anything incorrect in my approach, or a better way to achieve the same?


================
Comment at: test/Analysis/LossOfSignAssign.c:19
@@ +18,3 @@
+  return i+j; // implicit conversion here too!
+}
+
----------------
zaks.anna wrote:
> What happens if the return type is unsigned?
Do you think this would be something good to catch? I somehow assumed that this might be more noisy and less useful than the assignment cases?


http://reviews.llvm.org/D10634