[PATCH/RFC] Make -fanalyzer C only for GCC 10 (PR 93392)

[PATCH/RFC] Make -fanalyzer C only for GCC 10 (PR 93392)

[David Malcolm]

Although the analyzer works on GIMPLE SSA and therefore in theory ought
to support all languages supported by GCC, the code currently only
supports the subset of GIMPLE SSA expressible via the C frontend.

For GCC 10 I want to explicitly restrict the scope of the analyzer to
C code, to keep the initial scope of the feature sane.

For example, various C++ things aren't yet supported by the analyzer and
won't be in GCC 10 (exceptions, new/delete diagnostics, ctors that run
before main, etc)

There are already a couple of ICEs in BZ relating to -fanalyzer with
non-C code (PR 93288 and PR 93405, using C++ and Fortran respectively).

Rather than have the feature potentially crash if a user attempts to use
it on non C, I think it's more user-friendly to explicitly mark it as
C-only for the GCC 10 release.

I'm not sure of the ideal way to implement this.

This patch moves -fanalyzer from common.opt to c-family/c.opt, changing
it from "Common" to "C".

Unfortunately, doing it this way seems to mean losing LTO support, so
the patch also marks the LTO analyzer tests to be skipped.
LTO analysis currently does work when used on C code, albeit with some
UI warts, but there doesn't seem to be a good way to express
  "only use this in LTO with C"
and in theory a user could compile C++ to .o with -flto, and then link
with -fanalyzer etc - so it seems simplest to also drop link-time
-fanalyzer support for this release.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

OK for master?  Is there a better way to implement this?

Thanks
Dave


gcc/c-family/ChangeLog:
	PR analyzer/93392
	* c.opt (fanalyzer): Move here from common.opt, make C-specific.

gcc/ChangeLog:
	PR analyzer/93392
	* common.opt (fanalyzer): Move to c.opt.
	* doc/invoke.texi (-fanalyzer): Note that it is specific to the C
	front-end.

gcc/testsuite/ChangeLog:
	PR analyzer/93392
	* gcc.dg/analyzer/double-free-lto-1-a.c: Skip the test until LTO
	support can be re-enabled.
	* gcc.dg/analyzer/malloc-ipa-8-lto-c.c: Likewise.
	* gcc.dg/analyzer/torture/analyzer-torture.exp: Disable LTO for
	now when running torture tests.
---
 gcc/c-family/c.opt                                 |  4 ++++
 gcc/common.opt                                     |  4 ----
 gcc/doc/invoke.texi                                |  2 +-
 .../gcc.dg/analyzer/double-free-lto-1-a.c          |  3 +++
 gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c |  3 +++
 .../gcc.dg/analyzer/torture/analyzer-torture.exp   | 14 ++++++++++++++
 6 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 814ed17f7c4..da087f28f8e 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1303,6 +1303,10 @@ d
 C ObjC C++ ObjC++ Joined
 ; Documented in common.opt.  FIXME - what about -dI, -dD, -dN and -dD?
 
+fanalyzer
+C Var(flag_analyzer)
+Enable static analysis pass.
+
 fabi-compat-version=
 C++ ObjC++ Joined RejectNegative UInteger Var(flag_abi_compat_version) Init(-1)
 The version of the C++ ABI used for -Wabi warnings and link compatibility aliases.
diff --git a/gcc/common.opt b/gcc/common.opt
index 5692cd04374..e9b29fb4ee0 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -989,10 +989,6 @@ fallow-store-data-races
 Common Report Var(flag_store_data_races) Optimization
 Allow the compiler to introduce new data races on stores.
 
-fanalyzer
-Common Var(flag_analyzer)
-Enable static analysis pass.
-
 fargument-alias
 Common Ignore
 Does nothing. Preserved for backward compatibility.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 4dec0c8326b..a71992efcae 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8360,7 +8360,7 @@ Enabling this option effectively enables the following warnings:
 }
 
 This option is only available if GCC was configured with analyzer
-support enabled.
+support enabled, and is specific to the C front-end.
 
 @item -Wanalyzer-too-complex
 @opindex Wanalyzer-too-complex
diff --git a/gcc/testsuite/gcc.dg/analyzer/double-free-lto-1-a.c b/gcc/testsuite/gcc.dg/analyzer/double-free-lto-1-a.c
index 61e78467732..5e05e6aec9d 100644
--- a/gcc/testsuite/gcc.dg/analyzer/double-free-lto-1-a.c
+++ b/gcc/testsuite/gcc.dg/analyzer/double-free-lto-1-a.c
@@ -3,6 +3,9 @@
 /* { dg-additional-options "-flto" } */
 /* { dg-additional-sources double-free-lto-1-b.c } */
 
+/* For now, LTO support is disabled (PR analyzer/93392), so skip this test.  */
+/* { dg-skip-if "PR analyzer/93392" { *-*-* } } */
+
 #include <stdlib.h>
 #include "double-free-lto-1.h"
 
diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c
index d332db13ef0..03452c923db 100644
--- a/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c
+++ b/gcc/testsuite/gcc.dg/analyzer/malloc-ipa-8-lto-c.c
@@ -3,6 +3,9 @@
 /* { dg-additional-options "-flto" } */
 /* { dg-additional-sources "malloc-ipa-8-lto-a.c malloc-ipa-8-lto-b.c" } */
 
+/* For now, LTO support is disabled (PR analyzer/93392), so skip this test.  */
+/* { dg-skip-if "PR analyzer/93392" { *-*-* } } */
+
 #include <stdlib.h>
 #include "malloc-ipa-8-lto.h"
 
diff --git a/gcc/testsuite/gcc.dg/analyzer/torture/analyzer-torture.exp b/gcc/testsuite/gcc.dg/analyzer/torture/analyzer-torture.exp
index a4d98bb2297..0171d665a7a 100644
--- a/gcc/testsuite/gcc.dg/analyzer/torture/analyzer-torture.exp
+++ b/gcc/testsuite/gcc.dg/analyzer/torture/analyzer-torture.exp
@@ -23,6 +23,13 @@ if { ![check_effective_target_analyzer] } {
     return
 }
 
+# Disable LTO for now when running torture tests (PR analyzer/93392)
+global LTO_TORTURE_OPTIONS
+if [info exists LTO_TORTURE_OPTIONS] then {
+  set save_lto_torture_options $LTO_TORTURE_OPTIONS
+}
+set LTO_TORTURE_OPTIONS ""
+
 dg-init
 
 global DEFAULT_CFLAGS
@@ -42,3 +49,10 @@ if [info exists save_default_cflags] {
 } else {
   unset DEFAULT_CFLAGS
 }
+
+# Restore LTO_TORTURE_OPTIONS (PR analyzer/93392)
+if [info exists save_lto_torture_options] {
+  set LTO_TORTURE_OPTIONS $save_lto_torture_options
+} else {
+  unset LTO_TORTURE_OPTIONS
+}
-- 
2.21.0

Re: [PATCH/RFC] Make -fanalyzer C only for GCC 10 (PR 93392)

[Jakub Jelinek]

On Wed, Feb 05, 2020 at 09:59:19AM -0500, David Malcolm wrote:
> Although the analyzer works on GIMPLE SSA and therefore in theory ought
> to support all languages supported by GCC, the code currently only
> supports the subset of GIMPLE SSA expressible via the C frontend.
> 
> For GCC 10 I want to explicitly restrict the scope of the analyzer to
> C code, to keep the initial scope of the feature sane.
> 
> For example, various C++ things aren't yet supported by the analyzer and
> won't be in GCC 10 (exceptions, new/delete diagnostics, ctors that run
> before main, etc)

C has ctors before main too, look for attribute constructor.

I must say I don't really like this, if you encounter something in the IL
that the analyzer can't handle yet, punt, that is fine, but you could
write code in C++ that doesn't use exceptions nor new/delete, there is
no reason not to analyze that, etc.  And teaching it to handle new/delete
operator like malloc/free shouldn't be that hard, but even if you don't,
you still need some way how to deal with other allocators in C, say for
stuff allocated with mmap etc.

Mentioning in the documentation that it is for now primarily intended for C
is one thing (that is fine), but stopping analyzing something is another.

	Jakub

Re: [PATCH/RFC] Make -fanalyzer C only for GCC 10 (PR 93392)

[Jakub Jelinek]

On Wed, Feb 05, 2020 at 04:18:49PM +0100, Jakub Jelinek wrote:
> C has ctors before main too, look for attribute constructor.

And you can have exceptions in C too, attribute cleanup for the C
"destructors" that are invoked when exception is thrown or pthread
cancellation is invoked.

	Jakub

Re: [PATCH/RFC] Make -fanalyzer C only for GCC 10 (PR 93392)

[David Malcolm]

On Wed, 2020-02-05 at 16:18 +0100, Jakub Jelinek wrote:
> On Wed, Feb 05, 2020 at 09:59:19AM -0500, David Malcolm wrote:
> > Although the analyzer works on GIMPLE SSA and therefore in theory
> > ought
> > to support all languages supported by GCC, the code currently only
> > supports the subset of GIMPLE SSA expressible via the C frontend.
> > 
> > For GCC 10 I want to explicitly restrict the scope of the analyzer
> > to
> > C code, to keep the initial scope of the feature sane.
> > 
> > For example, various C++ things aren't yet supported by the
> > analyzer and
> > won't be in GCC 10 (exceptions, new/delete diagnostics, ctors that
> > run
> > before main, etc)
> 
> C has ctors before main too, look for attribute constructor.

Thanks.

> I must say I don't really like this, if you encounter something in
> the IL
> that the analyzer can't handle yet, punt, that is fine, 

Fair enough.

FWIW I managed to get double-free detection working in gfortran earlier
today, and in doing so discovered and fixed an issue affecting C,
proving your point, I think.

> but you could
> write code in C++ that doesn't use exceptions nor new/delete, there
> is
> no reason not to analyze that, etc.  And teaching it to handle
> new/delete
> operator like malloc/free shouldn't be that hard, but even if you
> don't,
> you still need some way how to deal with other allocators in C, say
> for
> stuff allocated with mmap etc.

Right.  I want to generalize the malloc/free stuff to deal with
arbitrary families of acquire/release APIs, but at this point we're
deep in stage 4, and so I want to try to set some expectations about
what's likely to work, and what isn't, and it seems prudent to leave
that generalization work to gcc 11.

My plan for gcc 10 is to focus on C code that uses malloc and free,
possibly with a simple wrapper around them.  I think the highest
priority item is ensuring that the analyzer scales up to handle real-
world C code rather than just the reduced examples in the testsuite. 
I'm working on a fix for a rather convoluted set of issues in state-
merging that would otherwise lead to the analyzer burning CPU cycles
w/o properly exploring the user's code.

But I'm also fixing ICEs as I go.

> Mentioning in the documentation that it is for now primarily intended
> for C
> is one thing (that is fine), but stopping analyzing something is
> another.

I'll do this in release notes and docs then.

Hope the above sounds like a reasonable plan.

Thanks
Dave