1156029 - Teach clang-analyzer about PR_ASSERT

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[(no longer active)]

Right now, clang-analyzer doesn't understand PR_ASSERT, so it generates a lot of false positives because it doesn't know about the assumptions made by the code through assertions.  I have a patch to fix that.

Comment 1

[(no longer active)]

Attachment: Teach clang-analyzer about PR_ASSERT

This patch makes clang-analyzer aware of the PR_ASSERT macro
if NSPR is compiled with scan-build.  It helps with suppressing
a large number of false positives found by clang-analyzer.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Fails to build for me:

/usr/share/clang/scan-build-3.7/ccc-analyzer -o prfdcach.o -c -fvisibility=hidden     -fno-omit-frame-pointer  -Wall -pthread -O2 -g -fno-inline -fPIC  -UNDEBUG -DDEBUG_jenkins -DPACKAGE_NAME=\"\" -DPACKAGE_TARNAME=\"\" -DPACKAGE_VERSION=\"\" -DPACKAGE_STRING=\"\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DMOZILLA_CLIENT=1 -DDEBUG=1 -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE=1 -DHAVE_VISIBILITY_PRAGMA=1 -DXP_UNIX=1 -D_GNU_SOURCE=1 -DHAVE_FCNTL_FILE_LOCKING=1 -DLINUX=1 -DHAVE_DLADDR=1 -DHAVE_LCHOWN=1 -DHAVE_SETPRIORITY=1 -DHAVE_STRERROR=1 -DHAVE_SYSCALL=1 -D_REENTRANT=1 -DFORCE_PR_LOG -D_PR_PTHREADS -UHAVE_CVAR_BUILT_ON_SEM -D_NSPR_BUILD_ -I/var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/dist/include/nspr -I/var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/include -I/var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/include/private  /var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/src/io/prfdcach.c
In file included from /var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/dist/include/nspr/pratom.h:14:0,
                 from /var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/dist/include/nspr/nspr.h:9,
                 from /var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/include/private/primpl.h:39,
                 from /var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/src/io/prfdcach.c:6:
/var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/dist/include/nspr/prtypes.h:530:20: error: missing binary operator before token "("
     __has_extension(attribute_analyzer_noreturn)
                    ^

Comment 3

[Wan-Teh Chang]

Comment on attachment 8594414 [details] [diff] [review]
Teach clang-analyzer about PR_ASSERT

Review of attachment 8594414 [details] [diff] [review]:
-----------------------------------------------------------------

::: nsprpub/pr/include/prtypes.h
@@ +510,5 @@
>  typedef unsigned long PRUword;
>  #endif
>  
> +/*
> + * PR_PRETEND_NORETURN_FOR_STATIC_ANALYSIS, specified at the end of a function

1. Nit: we can shorten this macro name a bit.

2. I think we should use this macro with PR_ProcessExit:
http://mxr.mozilla.org/nspr/ident?i=PR_ProcessExit

@@ +529,5 @@
> +#if defined(__clang_analyzer__) && \
> +    __has_extension(attribute_analyzer_noreturn)
> +#  define PR_PRETEND_NORETURN_FOR_STATIC_ANALYSIS __attribute__((analyzer_noreturn))
> +#else
> +#  define PR_PRETEND_NORETURN_FOR_STATIC_ANALYSIS /* no support */

In NSPR we don't indent these C preprocessor lines (lines 531 and 533).

Comment 4

[(no longer active)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> Fails to build for me:
> 
> /usr/share/clang/scan-build-3.7/ccc-analyzer -o prfdcach.o -c
> -fvisibility=hidden     -fno-omit-frame-pointer  -Wall -pthread -O2 -g
> -fno-inline -fPIC  -UNDEBUG -DDEBUG_jenkins -DPACKAGE_NAME=\"\"
> -DPACKAGE_TARNAME=\"\" -DPACKAGE_VERSION=\"\" -DPACKAGE_STRING=\"\"
> -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DMOZILLA_CLIENT=1 -DDEBUG=1
> -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE=1 -DHAVE_VISIBILITY_PRAGMA=1 -DXP_UNIX=1
> -D_GNU_SOURCE=1 -DHAVE_FCNTL_FILE_LOCKING=1 -DLINUX=1 -DHAVE_DLADDR=1
> -DHAVE_LCHOWN=1 -DHAVE_SETPRIORITY=1 -DHAVE_STRERROR=1 -DHAVE_SYSCALL=1
> -D_REENTRANT=1 -DFORCE_PR_LOG -D_PR_PTHREADS -UHAVE_CVAR_BUILT_ON_SEM
> -D_NSPR_BUILD_
> -I/var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/
> dist/include/nspr
> -I/var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/include
> -I/var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/include/private 
> /var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/src/io/prfdcach.c
> In file included from
> /var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/
> dist/include/nspr/pratom.h:14:0,
>                  from
> /var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/
> dist/include/nspr/nspr.h:9,
>                  from
> /var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/include/private/
> primpl.h:39,
>                  from
> /var/lib/jenkins/workspace/firefox-scan-build/nsprpub/pr/src/io/prfdcach.c:6:
> /var/lib/jenkins/workspace/firefox-scan-build/obj-x86_64-unknown-linux-gnu/
> dist/include/nspr/prtypes.h:530:20: error: missing binary operator before
> token "("
>      __has_extension(attribute_analyzer_noreturn)
>                     ^

What version of clang are you using?  That's really surprising since the same code is used on mozilla-central: <https://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#102>

Comment 5

[(no longer active)]

(In reply to Wan-Teh Chang from comment #3)
> ::: nsprpub/pr/include/prtypes.h
> @@ +510,5 @@
> >  typedef unsigned long PRUword;
> >  #endif
> >  
> > +/*
> > + * PR_PRETEND_NORETURN_FOR_STATIC_ANALYSIS, specified at the end of a function
> 
> 1. Nit: we can shorten this macro name a bit.

Do you have any suggestions?  I lifted this name from the one used in mfbt/Attributes.h, and I'm very bad at naming things myself. :-)

> 2. I think we should use this macro with PR_ProcessExit:
> http://mxr.mozilla.org/nspr/ident?i=PR_ProcessExit

Yes, but note that the reason this macro is useful is to help the static analysis tool interpret assertions.  For example, typically assert macros compile down to something like |if (!condition)ReportAssertion(...);|.  ReportAssertion is typically a function that calls aborts, and therefore doesn't return.  Static analysis tools look for patterns such as the above, and once they find one, they assume condition to be true, which helps with eliminating false positives.  For example, consider the code below, before and after this patch:

void f(int* a) {
  PR_ASSERT(a);
  printf("%d", *a);
}

Before this patch, the static analysis tool would report an error at the point that a is dereferenced, because it may be a null pointer.  However, after this patch, the compiler knows that if a is null, the program execution would be aborted before we get to the printf statement, so the static analysis would accept this code, which is probably what the programmer wants.

Because of the above, specifying this on functions such as PR_ProcessExit, although harmless, is not beneficial.

> @@ +529,5 @@
> > +#if defined(__clang_analyzer__) && \
> > +    __has_extension(attribute_analyzer_noreturn)
> > +#  define PR_PRETEND_NORETURN_FOR_STATIC_ANALYSIS __attribute__((analyzer_noreturn))
> > +#else
> > +#  define PR_PRETEND_NORETURN_FOR_STATIC_ANALYSIS /* no support */
> 
> In NSPR we don't indent these C preprocessor lines (lines 531 and 533).

OK, thank for the review!

Should I submit another patch here, or commit directly to the NSS repository?  (I'm not sure if L3 commit access includes NSPR and NSS...)

Comment 6

[Sylvestre Ledru [:Sylvestre]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4)
> What version of clang are you using?  That's really surprising since the
> same code is used on mozilla-central:
> <https://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#102>
A snapshot of 3.7 svn233711. I just updated to 3.7.0-svn235314-1~exp1 and it failed the same way.
scan-build runs gcc before during clang analyzer.

Comment 7

[(no longer active)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #4)
> > What version of clang are you using?  That's really surprising since the
> > same code is used on mozilla-central:
> > <https://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#102>
> A snapshot of 3.7 svn233711. I just updated to 3.7.0-svn235314-1~exp1 and it
> failed the same way.
> scan-build runs gcc before during clang analyzer.

gcc?!  Why do you need to run gcc?  I don't understand.  (FWIW this is off-topic for this bug...)

Comment 8

[Wan-Teh Chang]

Attachment: Teach clang-analyzer about PR_ASSERT, v2

Ehsan: I fixed the indentation in your patch.

To shorten the macro's name, I think either PR_PRETEND_NORETURN or
PR_ANALYZER_NORETURN is fine. Which one do you prefer?

Comment 9

[Wan-Teh Chang]

Ehsan: When compiling NSPR, I got the same compilation error that Sylvestre
reported in comment 2:

gcc -m32 -o prfdcach.o -c -fvisibility=hidden     -Wall -pthread -g -fno-inline -fPIC  -UNDEBUG -DDEBUG_wtc -DPACKAGE_NAME=\"\" -DPACKAGE_TARNAME=\"\" -DPACKAGE_VERSION=\"\" -DPACKAGE_STRING=\"\" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DDEBUG=1 -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE=1 -DHAVE_VISIBILITY_PRAGMA=1 -DXP_UNIX=1 -D_GNU_SOURCE=1 -DHAVE_FCNTL_FILE_LOCKING=1 -DLINUX=1 -Di386=1 -DHAVE_DLADDR=1 -DHAVE_LCHOWN=1 -DHAVE_SETPRIORITY=1 -DHAVE_STRERROR=1 -DHAVE_SYSCALL=1 -D_REENTRANT=1 -DFORCE_PR_LOG -D_PR_PTHREADS -UHAVE_CVAR_BUILT_ON_SEM -D_NSPR_BUILD_ -I../../../dist/include/nspr -I../../../../nspr/pr/include -I../../../../nspr/pr/include/private  ../../../../nspr/pr/src/io/prfdcach.c
In file included from ../../../dist/include/nspr/pratom.h:14:0,
                 from ../../../dist/include/nspr/nspr.h:9,
                 from ../../../../nspr/pr/include/private/primpl.h:39,
                 from ../../../../nspr/pr/src/io/prfdcach.c:6:
../../../dist/include/nspr/prtypes.h:529:20: error: missing binary operator before token "("
     __has_extension(attribute_analyzer_noreturn)
                    ^

I am using gcc:
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.8/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.8.2-19ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-4.8/README.Bugs --enable-languages=c,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.8 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.8 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --disable-libmudflap --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-4.8-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.8-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.8.2 (Ubuntu 4.8.2-19ubuntu1)

Comment 10

[Wan-Teh Chang]

Attachment: Teach clang-analyzer about PR_ASSERT, v2.1

We need to do something like this. See also
http://mxr.mozilla.org/mozilla-central/search?string=attribute_analyzer_noreturn

Comment 11

[(no longer active)]

Comment on attachment 8596230 [details] [diff] [review]
Teach clang-analyzer about PR_ASSERT, v2.1

Review of attachment 8596230 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!

Comment 12

[Wan-Teh Chang]

Comment on attachment 8596230 [details] [diff] [review]
Teach clang-analyzer about PR_ASSERT, v2.1

Review of attachment 8596230 [details] [diff] [review]:
-----------------------------------------------------------------

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/6f68148e104c