The previous patch had unrelated changes inadvertently. Corrected.

In D115919#3207280, @vitalybuka wrote:

hm, according to this it's Undefined Behavior
Sanitizer should probably crash in this case, at least with strict_string_checks
https://en.cppreference.com/w/c/string/byte/strstr

Instead, why should the sanitizer not diagnose the use of undefined behavior?
It would be good for the sanitizer to emit a diagnostic about "undefined behavior" (probably a warning). Code that leads to undefined behavior is generally not portable and is a bad programing practice that should be avoided.

In D10634#217438, @danielmarjamaki wrote:

In D10634#217433, @soumitra wrote:

In D10634#213835, @zaks.anna wrote:

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.

I am also still skeptic about this. Ideally there should only be warnings when there is a mistake.

In your example code you showed previously there were portability problems because the signedness changed. A warning for that is ok in my opinion.

If the code says 'unsigned int j = -1;' then there is no such portability problem.

Ah! Now I get it!!

In 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.

In D10634#201632, @danielmarjamaki wrote:

Here's a short test case that shows the problem:

Thanks!

I believe a fix for this problem would be a good thing. So that when this patch is added we don't get these false positives.

It should be a separate patch.

Sure, let me try to take a shot at it.

In D10634#199554, @danielmarjamaki wrote:

Here is a bigger log with more warnings.
https://drive.google.com/file/d/0BykPmWrCOxt2aGtRNTY4eXQ1OU0/view

there are some cases where I don't think there should be a warning.. for instance some compound assignments:

args.c:990:9: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

tmp /= 10;
    ^

read.c:4940:13: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

value >>= 7;
      ^

I am not an expert but it seems strange to me that clang seems to think that 10 and 7 are negative. Can you investigate this?

Here's a short test case that shows the problem:

1  void safe_mul(unsigned long x) {
2    unsigned long multiplier = 1000000000000000000; 
3    while (x > 0) {
4      multiplier /= 10;
5    }
6  }

In D10634#198447, @danielmarjamaki wrote:

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/abcm2ps/abcm2ps_7.8.9.orig.tar.gz
clang-tidy abcm2ps-7.8.9/deco.c
deco.c:788:9: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

ps_x = -1;

Only a temporary assignment, before being assigned to "signed char" ps_func, and hence safe.

Incorporated further review comments by Daniel.
Working copy updated, built, and tested using check-all.

Incorporated review comments by Daniel.
Updated revisions, rebuilt, retested using check-all.

Missed updating my working copy prior to creating the patch.
Updated, Rebuilt, and tested using check-all.

I had missed a check for initializer on a variable in my previous revision, resulting in a stack dump. Fixed.
Renamed the id and the test file as per Daniel's suggestion to reflect assignment.
Validated with make check-all, which I had missed earlier.

In D10634#192481, @danielmarjamaki wrote:

I recommend a more specific name than LossOfSignChecker unless you want it to have more warnings later. Right now, a name such as LossOfSignInAssignmentChecker.cpp would be better imho.

I don't have immediate plans to add more warnings on this, but there definitely are many more cases where we can lose the sign and potentially cause runtime failures.