[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D10634: Loss of Sign Checker
From:       Soumitra Chatterjee <soumitra () yahoo ! com>
Date:       2015-08-04 4:49:55
Message-ID: 36b1d30e710b80e1632d0491809a8a51 () localhost ! localdomain
[Download RAW message or body]

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




_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]