[PATCH] Loss of Sign Checker

[Daniel Marjamäki]

In http://reviews.llvm.org/D10634#192520, @soumitra wrote:

> In http://reviews.llvm.org/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.
>
> That said, I can rename it as you suggest since I do not have immediate plans to address more cases.


ok. I guess more warnings can be added later. So a generic name for the class is not a bad idea.

I'd recommend that you change the id in the td like:

  def LossOfSignChecker : Checker<"LossOfSignAssign">,

And then also rename the testfile to LossOfSignAssign.c.

> > Sure there can be issues with loss of sign. If we can warn about mistakes that is awesome!

> 

> > 

> 

> > However personally I think it seems noisy to warn about:

> 

> > 

> 

> >   unsigned char uc1 = -1;

> 

> > 

> 

> > 

> 

> > As I see it, it is quite clear that the developer wants to assign -1 to uc1 and that a cast is expected. On a normal machine the uc1 would get the value 255 and this is likely expected.

> 

> 

> Sure, but wouldn't it be better to expect an explicit cast if the developer really meant what he wrote?


Well.. I am not sure what the Clang users thinks about this. Maybe they would accept changing their code. Personally I'd prefer:

  unsigned char = ~0U;

It would be nice to see some actual warnings and see if there are possible mistakes.

> In http://reviews.llvm.org/D10634#192482, @danielmarjamaki wrote:

> 

> > Have you tried this checker on some real projects to see if the results are good?

> 

> 

> No, not really.


ok. I can run it on some projects.. I have a testscript I run.

> However, I hit this issue when moving a large software from x86 to AArch64, where unfortunately, the 'sign' of a plain char changes, causing us to lose days of effort in narrowing down the issue.

> 

> For example, the following representative program that works perfectly on x86 would fail silently on AArch64:

> 

>    1  #include <stdio.h>

>    2

>    3  const char data[] = { -1 };

>    4

>    5  int main() {

>    6     int value = data[0];

>    7     if (value != -1) {

>    8        printf("failed! value = %d\n", value);

>    9        return 1;

>   10     }

>   11     return 0;

>   12  }

>    


Nice example.

> Do you think narrowing down the checker to the above singular case would be more appropriate? I would still think that while the 'unsigned char' case you point out may be a grey area, an "integer" assignment with the wrong sign would certainly not be a good programming practice.


I would definitely feel better about this then.

But I will try your current patch on some open source projects.. and then we will have some actual results to look at.

Feel free to run your patch on some projects too.


http://reviews.llvm.org/D10634

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/