This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Fix logical not for pointers with different bit width
ClosedPublic

Authored by danielmarjamaki on Mar 16 2017, 6:48 AM.

Details

Summary

The Static Analyzer assumed that all pointers had the same bit width. Now pass the type to the 'makeNull' method, to construct a null
pointer of the appropiate bit width.

Example code that does not work well:

int main(void) {
  __cm void *cm_p = 0;
  if (cm_p == 0)
    (void)cm_p;
}

Unfortunately there is no proper testcase here. The problem is seen with a custom target.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun edited edge metadata.Mar 16 2017, 7:26 AM

So there is no LLVM supported target with different bit width pointer types?

In case we have such a target upstreamed, you can add a test with the host-triple hardcoded into the test.

I am not sure where to look. I heard somebody say OpenCL has different pointer widths.

grandinj added inline comments.
include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
318–322

spelling and grammar and doxygen formatting:

/// \param type accommodate different pointer bit-widths of different address spaces.

zaks.anna edited edge metadata.Mar 16 2017, 5:26 PM

Are there other cases where makeNull would need to be replaced?

dkrupp added a subscriber: dkrupp.Mar 22 2017, 2:49 AM

Added a testcase that will crash without the fix. Used the amdgcn target as that happens to use different pointer bit widths for different address spaces.

Updated the comment. I am not good at english so I hope the grammar is ok.

Are there other cases where makeNull would need to be replaced?

There might be. As I understand it, this is the only known case at the moment.

danielmarjamaki marked an inline comment as done.Mar 23 2017, 9:05 AM

Are there other cases where makeNull would need to be replaced?

There might be. As I understand it, this is the only known case at the moment.

To clarify. The static analyser runs fine on plenty of code. I ran CSA using this target on a 1000's of C-files project. I think it works well.. but I can't guarantee there won't be any more issues.

Sorry for the delay!!!

include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
184

The isUnsigned parameter is not used and is not necessary as the type itself has this info.

Not clear if this function is expected to work for all types or only scalar types (maybe assert that the input is isScalarType()).

Fix review comments

danielmarjamaki marked an inline comment as done.May 16 2017, 1:28 AM
zaks.anna accepted this revision.Jun 14 2017, 3:16 PM

Looks good with a nit!

include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
325

Formatting changes here look inconsistent with the rest of the code around.

This revision is now accepted and ready to land.Jun 14 2017, 3:16 PM
This revision was automatically updated to reflect the committed changes.
danielmarjamaki marked an inline comment as done.