Re: replacing the backwards threader and more

[Richard Biener via Gcc]

On Fri, Jun 25, 2021 at 6:20 PM Aldy Hernandez  wrote:
>
> Hi folks.
>
> I'm done with benchmarking, testing and cleanups, so I'd like to post my
> patchset for review.  However, before doing so, I'd like to address a
> handful of meta-issues that may affect how I post these patches.

First of all thanks for the detailed analysis and report!

> Trapping on differences
> ===
>
> Originally I wanted to contribute verification code that would trap if
> the legacy code threaded any edges the new code couldn't (to be removed
> after a week).  However, after having tested on various architectures
> and only running once into a missing thread, I'm leaning towards
> omitting the verification code, since it's fragile, time consuming, and
> quite hacky.

Agreed.

> For the record, I have tested on x86-64, aarch64, ppc64 and ppc64le.
> There is only one case, across bootstrap and regression tests where the
> verification code is ever tripped (discussed below).
>
> Performance
> ===
>
> I re-ran benchmarks as per our callgrind suite, and the penalty with the
> current pipeline is 1.55% of overall compilation time.  As is being
> discussed, we should be able to mitigate this significantly by removing
> other threading passes.

1.55% for the overall compilation looks quite large - is this suite particularly
demanding on VRP/ranger?  Is the figure for 'cc1files' similar? (collect
preprocessed sources of gcc/*.{c,cc} compiles, like by appending
-save-temps to CFLAGS in a non-bootstrap build)

I'm curious if you tried to look at the worst offenders and if there's
anything new, threading specific.  More threading might result in
larger code (how is cc1 size affected by the change?) and more
code to compile by later passes can easily explain >1% differences.

> Failing testcases
> =
>
> I have yet to run into incorrect code being generated, but I have had to
> tweak a considerable number of tests.  I have verified every single
> discrepancy and documented my changes in the testsuite when it merited
> doing so.  However, there are a couple tests that trigger regressions
> and I'd like to ask for guidance on how to address them.
>
> 1. gcc.c-torture/compile/pr83510.c
>
> I would like to XFAIL this.
>
> What happens here is that thread1 threads a switch statement such that
> the various cases have been split into different independent blocks.
> One of these blocks exposes an arr[i_27] access which is later
> propagated by VRP to be arr[10].  This is an invalid access, but the
> array bounds code doesn't know it is an unreachable path.
>
> However, it is not until dom2 that we "know" that the value of the
> switch index is such that the path to arr[10] is unreachable.  For that
> matter, it is not until dom3 that we remove the unreachable path.

Sounds reasonable.

> 2. -Wfree-nonheap-object
>
> This warning is triggered while cleaning up an auto_vec.  I see that the
> va_heap::release() inline is wrapped with a pragma ignore
> "-Wfree-nonheap-object", but this is not sufficient because jump
> threading may alter uses in such a way that may_emit_free_warning() will
> warn on the *inlined* location, thus bypassing the pragma.
>
> I worked around this with a mere:
>
>  > @@ -13839,6 +13839,7 @@ maybe_emit_free_warning (tree exp)
> >location_t loc = tree_inlined_location (exp);
> > +  loc = EXPR_LOCATION (exp);
>
> but this causes a ton of Wfree-nonheap* tests to fail.  I think someone
> more knowledgeable should address this (msebor??).

That looks wrong, but see msebors response for maybe some better answer.

> 3. uninit-pred-9_b.c
>
> The uninit code is getting confused with the threading and the bogus
> warning in line 24 is back.  I looked at the thread, and it is correct.
>
> I'm afraid all these warnings are quite fragile in the presence of more
> aggressive optimizations, and I suspect it will only get worse.

Yep.  Shouldn't be a blocker, just XFAIL ...

> 4. libphobos/src/std/net/isemail.d
>
> This is a D test where we don't actually fail, but we trigger the
> verification code.  It is the only jump threading edge that the new code
> fails to get over the old code, and it only happens on ppc64.
>
> It triggers because a BB4 -> BB5 is too expensive to thread, but a BBn
> -> BB3 -> BB4 -> BB5 is considered safe to thread because BB3 is a latch
> and it alters the profitability equation.  The reason we don't get it,
> is that we assume that if a X->Y is unprofitable, it is not worth
> looking at W->X->Y and so forth.
>
> Jeff had some fancy ideas on how to attack this.  Once such idea was to
> stop looking back, but only for things we were absolutely sure would
> never yield a profitable path.  I tried a subset of this, by allowing
> further looks on this latch test, but my 1.55% overall performance
> penalty turned into an 8.33% penalty.  Personally it looks way too
> expensive for this one isolated case.  Besides, the test where this
> clamping code originally came from still

Re: GCC documentation: porting to Sphinx

[Arnaud Charlet]

> I've got something that is very close to be a patch candidate that can be
> eventually merged. Right now, the patches are available here:
> https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;a=log;h=refs/users/marxin/heads/sphinx-v3

FWIW I would prefer to review the changes posted here directly with all
the details.

In particular can you explain the motivation behind all the changes in the
gcc/ada/doc directory? That's lots of moving files around, so I'd like
to understand why and make sure these are not gratuituous changes, since
move files around is always tricky and I'd rather not have to undo it
later in case this causes troubles or have unexpected consequences.

Otherwise, glad to see the switch to sphinx finally moving in gcc!

Arno

Re: GCC documentation: porting to Sphinx

[Martin Liška]

On 6/28/21 12:23 PM, Arnaud Charlet wrote:

I've got something that is very close to be a patch candidate that can be
eventually merged. Right now, the patches are available here:
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;a=log;h=refs/users/marxin/heads/sphinx-v3


FWIW I would prefer to review the changes posted here directly with all
the details.


Sure, I'm going to send a proper patch set in an hour or so. As mentioned, I 
won't be able to attach
some of the patches as they will exceed 1MB email limit.



In particular can you explain the motivation behind all the changes in the
gcc/ada/doc directory?


Sure:
1) All Sphinx manuals live in a directory where index page is called index.rst. 
That's why
I moved e.g. this: gcc/ada/doc/{gnat_rm.rst => gnat_rm/index.rst}
2) I moved latex_elements.py to ada_latex_elements.py as it clashes with Sphinx 
global variable
you modify in Sphinx config files
3) I created a shared Ada config (adabaseconf.py) that extends doc/baseconf.py 
and sets what
is shared in between 3 Ada manuals.
4) gnu_free_documentation_license.rst is taken from $root/doc/


That's lots of moving files around, so I'd like
to understand why and make sure these are not gratuituous changes, since
move files around is always tricky and I'd rather not have to undo it
later in case this causes troubles or have unexpected consequences.


Hope I explained all the reasonable changes?



Otherwise, glad to see the switch to sphinx finally moving in gcc!


You're welcome. I would be interested in testing your PRO configuration (based 
on Gnat_Build_Type,
see get_gnat_build_type) and I'm curious if you're fine with Sphinx template 
change?
It will be the same as for other manuals.

Cheers,
Martin



Arno

GCC Rust Monthly Call - 2nd July 2021

[Philip Herron]

Hi everyone,

It is that time again and we will be having our 4th community call over
on Jitsi for the first Friday of the month.

  * Date: 2nd July 2021
  o UTC: 0900
  o UK/Ireland UTC+1/BST: 1000
  o Central European Summer Time UTC+2: 1100
  * Monthly Report: WIP
  * Agenda (WIP): https://hackmd.io/A-b-G90YTNSCEVjiHXz7Qg

  * Jitsi video call: https://meet.jit.si/259057065581073


Here is the latest weekly report for the project:
https://github.com/Rust-GCC/Reporting/blob/main/2021-06-21-report.org

Please feel free to suggest any topics/questions you wish to be
discussed here or directly onto the agenda hackmd document.

Thanks

--Phil



OpenPGP_signature
Description: OpenPGP digital signature

[PATCH] Port GCC documentation to Sphinx

[Martin Liška]

Hello.

I'm sending the complete patch set that includes ChangeLog entries. 
Unfortunately,
majority of the patches are huge, that's why I sent like to a tarball:
https://splichal.eu/tmp/port-to-sphinx-v1.tar

The tarball contains the following patches:

19e06194746 Ada: port to Sphinx.
9a744ca431d Remove unused TEX files.
e624967b5e8 Port jit to new Sphinx layout.
8c4717b262a Build system: support Sphinx
d102880437e Add include directives for target macros.
08c3d3f0d8d Add RST files with config files.

Thanks,
Martin

Re: replacing the backwards threader and more

[Aldy Hernandez via Gcc]

On 6/28/21 10:22 AM, Richard Biener wrote:

On Fri, Jun 25, 2021 at 6:20 PM Aldy Hernandez  wrote:


Hi folks.

I'm done with benchmarking, testing and cleanups, so I'd like to post my
patchset for review.  However, before doing so, I'd like to address a
handful of meta-issues that may affect how I post these patches.


First of all thanks for the detailed analysis and report!


Trapping on differences
===

Originally I wanted to contribute verification code that would trap if
the legacy code threaded any edges the new code couldn't (to be removed
after a week).  However, after having tested on various architectures
and only running once into a missing thread, I'm leaning towards
omitting the verification code, since it's fragile, time consuming, and
quite hacky.


Agreed.


For the record, I have tested on x86-64, aarch64, ppc64 and ppc64le.
There is only one case, across bootstrap and regression tests where the
verification code is ever tripped (discussed below).

Performance
===

I re-ran benchmarks as per our callgrind suite, and the penalty with the
current pipeline is 1.55% of overall compilation time.  As is being
discussed, we should be able to mitigate this significantly by removing
other threading passes.


1.55% for the overall compilation looks quite large - is this suite particularly
demanding on VRP/ranger?  Is the figure for 'cc1files' similar? (collect
preprocessed sources of gcc/*.{c,cc} compiles, like by appending
-save-temps to CFLAGS in a non-bootstrap build)


The figures are for cc1 files.  They are .ii files we run under 
callgrind and compare "instruction counts" as per the tool.  We've found 
it's comparable to -ftime-report, though not exactly.  We've been using 
this for benchmarking, since evrp times were so small that it was hard 
to compare user times.




I'm curious if you tried to look at the worst offenders and if there's
anything new, threading specific.  More threading might result in
larger code (how is cc1 size affected by the change?) and more
code to compile by later passes can easily explain >1% differences.


A while ago, I did look at worst offenders on the branch, but there was 
nothing obvious.  As it stands now, the average cost for the threading 
pass is 858%, with 95% of .ii files falling under 1800%.  I could look 
at the outliers which seem to be c-fold, gcc-ar, gcc-ranlib, gcc-nm, and 
c-opts.


cc1 on x86-64 seems to be 0.135% larger for an --enable-checking build.




Failing testcases
=

I have yet to run into incorrect code being generated, but I have had to
tweak a considerable number of tests.  I have verified every single
discrepancy and documented my changes in the testsuite when it merited
doing so.  However, there are a couple tests that trigger regressions
and I'd like to ask for guidance on how to address them.

1. gcc.c-torture/compile/pr83510.c

I would like to XFAIL this.

What happens here is that thread1 threads a switch statement such that
the various cases have been split into different independent blocks.
One of these blocks exposes an arr[i_27] access which is later
propagated by VRP to be arr[10].  This is an invalid access, but the
array bounds code doesn't know it is an unreachable path.

However, it is not until dom2 that we "know" that the value of the
switch index is such that the path to arr[10] is unreachable.  For that
matter, it is not until dom3 that we remove the unreachable path.


Sounds reasonable.


2. -Wfree-nonheap-object

This warning is triggered while cleaning up an auto_vec.  I see that the
va_heap::release() inline is wrapped with a pragma ignore
"-Wfree-nonheap-object", but this is not sufficient because jump
threading may alter uses in such a way that may_emit_free_warning() will
warn on the *inlined* location, thus bypassing the pragma.

I worked around this with a mere:

  > @@ -13839,6 +13839,7 @@ maybe_emit_free_warning (tree exp)

location_t loc = tree_inlined_location (exp);
+  loc = EXPR_LOCATION (exp);


but this causes a ton of Wfree-nonheap* tests to fail.  I think someone
more knowledgeable should address this (msebor??).


That looks wrong, but see msebors response for maybe some better answer.


Oh, it was just a stop-gap.  I have instead opted for the following 
temporary measure in Makefile.in:


tree-ssa-loop-im.o-warn = -Wno-free-nonheap-object

I will work with Martin on a more appropriate solution.




3. uninit-pred-9_b.c

The uninit code is getting confused with the threading and the bogus
warning in line 24 is back.  I looked at the thread, and it is correct.

I'm afraid all these warnings are quite fragile in the presence of more
aggressive optimizations, and I suspect it will only get worse.


Yep.  Shouldn't be a blocker, just XFAIL ...


4. libphobos/src/std/net/isemail.d

This is a D test where we don't actually fail, but we trigger the
verification code.  It is the only jump threading edge that the new code
fails to get over the old

Re: Unit Type for Rust

[Philip Herron]

On 28/06/2021 14:49, Philip Herron wrote:
> Hi everyone,
>
> In Rust the language has the notion of the unit type '()', so for example:
>
>  fn foo ->i32 { ... }
>  fn bar() { ... }
>
> Foo has the return type i32, and bar has no return type, which means it
> is unit-type so that it can be a value assignable just like any other
> function call. You can also declare unit-structs or unit as a type on
> variables:
>
>  struct foobar; // empty unit struct
>  let a:() = (); // unit type
>
> I thought I could use GCC's void_type_node to represent the unit type
> and void_node for the value when assigning or using it, but this causes
> the ICE:
>
> ```
> In function ‘test’:
> rust1: internal compiler error: in create_tmp_var, at gimple-expr.c:482
> 0x13fd3bf create_tmp_var(tree_node*, char const*)
>     ../../gccrs/gcc/gimple-expr.c:482
> 0xe5d195 Gcc_backend::temporary_variable(Bfunction*, Bblock*, Btype*,
> Bexpression*, bool, Location, Bstatement**)
>     ../../gccrs/gcc/rust/rust-gcc.cc:2889
> 0xfe3479 Rust::HIR::Function::accept_vis(Rust::HIR::HIRVisitor&)
>     ../../gccrs/gcc/rust/hir/tree/rust-hir-full-test.cc:4414
> 0xf95cdb Rust::Compile::CompileCrate::go()
>     ../../gccrs/gcc/rust/backend/rust-compile.cc:49
> 0xf95b8b Rust::Compile::CompileCrate::Compile(Rust::HIR::Crate&,
> Rust::Compile::Context*)
>     ../../gccrs/gcc/rust/backend/rust-compile.cc:39
> 0xee92e7 Rust::Session::parse_file(char const*)
>     ../../gccrs/gcc/rust/rust-session-manager.cc:596
> 0xee8d76 Rust::Session::parse_files(int, char const**)
>     ../../gccrs/gcc/rust/rust-session-manager.cc:459
> 0xe45264 grs_langhook_parse_file
>     ../../gccrs/gcc/rust/rust-lang.cc:171
>
> ```
>
> I think because void_node is likely not COMPLETE_TYPE_P which means it
> hits the assertion when you need temporary's.
>
>
> Then Tom Tromey suggested I try a zero precision integer so I called:
> make_unsigned_type (0) for the type and then use integer_zero_node for
> the value, and this solves the problem; however, if I use this zero
> precision integer type for the return type on functions and turn
> optimizations on I get the ICE:
>
>    ```
>    test.rs: In function ‘main’:
>    test.rs:16:1: internal compiler error: in min_value, at wide-int.cc:346
>   16 | fn main() {
>  | ^
>    0x1d551d5 wi::min_value(unsigned int, signop)
>    ../../gccrs/gcc/wide-int.cc:346
>    0x1146ca5 irange::set_varying(tree_node*)
>    ../../gccrs/gcc/value-range.h:476
>    0x1ce5970 value_range_equiv::set_varying(tree_node*)
>    ../../gccrs/gcc/value-range-equiv.cc:71
>    0x1d3da07 vr_values::set_def_to_varying(tree_node const*)
>    ../../gccrs/gcc/vr-values.c:230
>    0x1d3da70 vr_values::set_defs_to_varying(gimple*)
>    ../../gccrs/gcc/vr-values.c:241
>    0x1c78b2f vrp_prop::visit_stmt(gimple*, edge_def**, tree_node**)
>    ../../gccrs/gcc/tree-vrp.c:4001
>    0x1ad8519 ssa_propagation_engine::simulate_stmt(gimple*)
>    ../../gccrs/gcc/tree-ssa-propagate.c:230
>    0x1ad8a0e ssa_propagation_engine::simulate_block(basic_block_def*)
>    ../../gccrs/gcc/tree-ssa-propagate.c:337
>    0x1ad9f2e ssa_propagation_engine::ssa_propagate()
>    ../../gccrs/gcc/tree-ssa-propagate.c:800
>    0x1c7a0b0 execute_vrp
>    ../../gccrs/gcc/tree-vrp.c:4512
>    0x1c7a3e4 execute
>    ../../gccrs/gcc/tree-vrp.c:4620
>    Please submit a full bug report,
>    ```
>
> The backtrace looks as though the optimizer is looking for min value for
> a default for the return value, but it's a zero precision integer that
> hits the assertion. Note running with -O0, the assertion does not get hit.
>
>
> At the moment, I have left functions with return type unit to keep using
> void_type_node and everywhere else, use this new zero precision integer.
> I am not sure what the best approach is here; I was hoping to solicit
> feedback on what I am doing with the folks here on the mailing list.
>
> Thanks
>
> --Phil
>

Adding in the GCC mailing list for feedback.

Thanks

--Phil



OpenPGP_signature
Description: OpenPGP digital signature

Re: daily report on extending static analyzer project [GSoC]

[Ankur Saini via Gcc]

> On 28-Jun-2021, at 12:18 AM, David Malcolm  wrote:
>> 
>>> 
 
 Q. But even if we find out which function to call, how will the
 analyzer know which snode does that function belong ?
>>> 
>>> Use this method of supergraph:
>>>  supernode *get_node_for_function_entry (function *fun) const;
>>> to get the supernode for the entrypoint of a given function.
>>> 
>>> You can get the function * from a fndecl via DECL_STRUCT_FUNCTION.
>> 
>> so once we get fndecl, it should be comparatively smooth sailing from
>> there. 
>> 
>> My attempt to get the value of function pointer from the state : -
>> 
>> - to access the region model of the state, I tried to access
>> “m_region_model” of that state.
>> - now I want to access cluster for a function pointer.
>> - but when looking at the accessible functions to region model class,
>> I couldn’t seem to find the fitting one. ( the closest I could find
>> was “region_model::get_reachable_svalues()” to get a set of all the
>> svalues reachable from that model )
> 
> In general you can use:
>  region_model::get_rvalue
> to go from a tree to a symbolic value for what the analyzer "thinks"
> the value of that tree is at that point along the path.
> 
> If it "knows" that it's a specific function pointer, then IIRC this
> will return a region_svalue where region_svalue::get_pointee () will
> (hopefully) point at the function_region representing the memory
> holding the code of the function.  function_region::get_fndecl should
> then give you the tree for the specific FUNCTION_DECL, from which you
> can find the supergraph node etc.
> 
> It looks like
>  region_model::get_fndecl_for_call
> might already do most of what you need, but it looks like it bails out
> for the "NULL cgraph_node" case.  Maybe that needs fixing, so that it
> returns the fndecl for that case?  That already gets used in some
> places, so maybe try putting a breakpoint on that and see if fixing
> that gets you further?

shouldn’t the fn_decl should still have a cgraph_node if the function is 
declared in the program itself ? it should just not have an edge representing 
the call.
Because I was able to find the super-graph node just with the help of the 
function itself.

this is how the function looks "exploded_node::on_edge()" right now.

File: {$SCR_DIR}/gcc/analyzer/engine.cc
1305: bool
1306: exploded_node::on_edge (exploded_graph &eg,
1307:   const superedge *succ,
1308:   program_point *next_point,
1309:   program_state *next_state,
1310:   uncertainty_t *uncertainty)
1311: {
1312:   LOG_FUNC (eg.get_logger ());
1313: 
1314:   if (succ->m_kind == SUPEREDGE_INTRAPROCEDURAL_CALL)
1315:   {
1316: const program_point *this_point = &this->get_point();
1317: const program_state *this_state = &this->get_state ();
1318: const gcall *call = this_point->get_supernode ()->get_final_call 
();
1319: 
1320: impl_region_model_context ctxt (eg, 
1321:   this, 
1322:   this_state, 
1323:   next_state, 
1324:   uncertainty,
1325:   this_point->get_stmt());
1326: 
1327: region_model *model = this_state->m_region_model;
1328: tree fn_decl = model->get_fndecl_for_call(call,&ctxt);
1329: if(DECL_STRUCT_FUNCTION(fn_decl))
1330: {
1331:   const supergraph *sg = &eg.get_supergraph();
1332:   supernode * sn =  sg->get_node_for_function_entry 
(DECL_STRUCT_FUNCTION(fn_decl));
1333:   // create enode and eedge ?
1334: }
1335:   }
1336: 
1337:   if (!next_point->on_edge (eg, succ))
1338: return false;
1339: 
1340:   if (!next_state->on_edge (eg, this, succ, uncertainty))
1341: return false;
1342: 
1343:   return true;
1344: }

for now, it is also detecting calls that already have call_sedge connecting 
them, so I think I also have to filter them out.

> 
> Hope this is helpful
> Dave

Thanks 
- Ankur

Re: [PATCH] Port GCC documentation to Sphinx

[Joseph Myers]

Are formatted manuals (HTML, PDF, man, info) corresponding to this patch 
version also available for review?

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Unit Type for Rust

[Segher Boessenkool]

On Mon, Jun 28, 2021 at 02:51:22PM +0100, Philip Herron wrote:
> On 28/06/2021 14:49, Philip Herron wrote:
> > In Rust the language has the notion of the unit type '()', so for example:
> >
> >  fn foo ->i32 { ... }
> >  fn bar() { ... }
> >
> > Foo has the return type i32, and bar has no return type, which means it
> > is unit-type so that it can be a value assignable just like any other
> > function call. You can also declare unit-structs or unit as a type on
> > variables:
> >
> >  struct foobar; // empty unit struct
> >  let a:() = (); // unit type

So you probably should have the unit type only in the language code, and
use an actual type for anything later?  Or can the actual type stay
unknown as well?  That is new territory for GCC.

> > I thought I could use GCC's void_type_node to represent the unit type
> > and void_node for the value when assigning or using it, but this causes
> > the ICE:

"void" already has a meaning, and it is not this.

> > Then Tom Tromey suggested I try a zero precision integer so I called:
> > make_unsigned_type (0) for the type and then use integer_zero_node for
> > the value, and this solves the problem; however, if I use this zero
> > precision integer type for the return type on functions and turn
> > optimizations on I get the ICE:
> >
> >    ```
> >    test.rs: In function ‘main’:
> >    test.rs:16:1: internal compiler error: in min_value, at wide-int.cc:346

So you'll need to update the max/min code to know about this.  Or, don't
expose unit type in generic code at all?  Can the language code not
always lower it to an actual type instead of the placeholder it is?

There will be many more places things will break after vrp1.  Including
many RTL passes.


Segher

Using source-level annotations to help GCC detect buffer overflows

[Martin Sebor via Gcc]

I wrote an article for the Red Hat Developer blog about how
to annotate code to get the most out of GCC's access checking
warnings like -Warray-bounds, -Wformat-overflow, and
-Wstringop-overflow.  The article published last week:

https://developers.redhat.com/articles/2021/06/25/use-source-level-annotations-help-gcc-detect-buffer-overflows

Martin

Re: replacing the backwards threader and more

[Martin Sebor via Gcc]

On 6/28/21 12:17 AM, Aldy Hernandez wrote:



On 6/25/21 7:19 PM, Martin Sebor wrote:

On 6/25/21 10:20 AM, Aldy Hernandez via Gcc wrote:

Hi folks.

I'm done with benchmarking, testing and cleanups, so I'd like to post 
my patchset for review.  However, before doing so, I'd like to 
address a handful of meta-issues that may affect how I post these 
patches.


Trapping on differences
===

Originally I wanted to contribute verification code that would trap 
if the legacy code threaded any edges the new code couldn't (to be 
removed after a week).  However, after having tested on various 
architectures and only running once into a missing thread, I'm 
leaning towards omitting the verification code, since it's fragile, 
time consuming, and quite hacky.


For the record, I have tested on x86-64, aarch64, ppc64 and ppc64le. 
There is only one case, across bootstrap and regression tests where 
the verification code is ever tripped (discussed below).


Performance
===

I re-ran benchmarks as per our callgrind suite, and the penalty with 
the current pipeline is 1.55% of overall compilation time.  As is 
being discussed, we should be able to mitigate this significantly by 
removing other threading passes.


Failing testcases
=

I have yet to run into incorrect code being generated, but I have had 
to tweak a considerable number of tests.  I have verified every 
single discrepancy and documented my changes in the testsuite when it 
merited doing so.  However, there are a couple tests that trigger 
regressions and I'd like to ask for guidance on how to address them.


1. gcc.c-torture/compile/pr83510.c

I would like to XFAIL this.

What happens here is that thread1 threads a switch statement such 
that the various cases have been split into different independent 
blocks. One of these blocks exposes an arr[i_27] access which is 
later propagated by VRP to be arr[10].  This is an invalid access, 
but the array bounds code doesn't know it is an unreachable path.


The test has a bunch of loops that iterate over the 10 array elements.
There have been bug reports about loop unrolling causing false positives
-Warray-bounds (e.g., PR 92539, 92110, or 86341) so this could be
the same issue.



However, it is not until dom2 that we "know" that the value of the 
switch index is such that the path to arr[10] is unreachable.  For 
that matter, it is not until dom3 that we remove the unreachable path.


If you do XFAIL it can you please isolate a small test case and open
a bug and make it a -Warray-bounds blocker?


Will do.





2. -Wfree-nonheap-object

This warning is triggered while cleaning up an auto_vec.  I see that 
the va_heap::release() inline is wrapped with a pragma ignore 
"-Wfree-nonheap-object", but this is not sufficient because jump 
threading may alter uses in such a way that may_emit_free_warning() 
will warn on the *inlined* location, thus bypassing the pragma.


I worked around this with a mere:

 > @@ -13839,6 +13839,7 @@ maybe_emit_free_warning (tree exp)

   location_t loc = tree_inlined_location (exp);
+  loc = EXPR_LOCATION (exp);


but this causes a ton of Wfree-nonheap* tests to fail.  I think 
someone more knowledgeable should address this (msebor??).


This sounds like the same problem as PR 98871.  Does the patch below
fix it?
https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572515.html
If so, I suggest getting that patch in first to avoid testsuite
failures.  If it doesn't fix it I'll look into it before you commit
your changes.


The latest patch in the above thread does not apply.  Do you have a more 
recent patch?


The first patch in the series applies cleanly for me:
  https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572516.html
but patch 2/4 does need a few adjustments.  With those and your
latest changes applied I don't see any -Wfree-nonheap-object test
failures.

Martin




3. uninit-pred-9_b.c

The uninit code is getting confused with the threading and the bogus 
warning in line 24 is back.  I looked at the thread, and it is correct.


I'm afraid all these warnings are quite fragile in the presence of 
more aggressive optimizations, and I suspect it will only get worse.


 From my recent review of open -Wmaybe-uninitialized bugs (and
the code) it does seem to be both fragile and getting worse.  I've
only found a few simple problems so far in the code but nothing that
would make a dramatic difference so I can't say if it's possible to
do much better, but I'm not done or ready to give up.  If you XFAIL
this too please open a bug for it and make it a blocker for
-Wuninitialized?


Will do.

Good to know these are somewhat known issues.

Thanks.
Aldy

Re: daily report on extending static analyzer project [GSoC]

[David Malcolm via Gcc]

On Mon, 2021-06-28 at 20:23 +0530, Ankur Saini wrote:
> 
> 
> > On 28-Jun-2021, at 12:18 AM, David Malcolm 
> > wrote:
> > > 
> > > > 
> > > > > 
> > > > > Q. But even if we find out which function to call, how will
> > > > > the
> > > > > analyzer know which snode does that function belong ?
> > > > 
> > > > Use this method of supergraph:
> > > >  supernode *get_node_for_function_entry (function *fun) const;
> > > > to get the supernode for the entrypoint of a given function.
> > > > 
> > > > You can get the function * from a fndecl via
> > > > DECL_STRUCT_FUNCTION.
> > > 
> > > so once we get fndecl, it should be comparatively smooth sailing
> > > from
> > > there. 
> > > 
> > > My attempt to get the value of function pointer from the state :
> > > -
> > > 
> > > - to access the region model of the state, I tried to access
> > > “m_region_model” of that state.
> > > - now I want to access cluster for a function pointer.
> > > - but when looking at the accessible functions to region model
> > > class,
> > > I couldn’t seem to find the fitting one. ( the closest I could
> > > find
> > > was “region_model::get_reachable_svalues()” to get a set of all
> > > the
> > > svalues reachable from that model )
> > 
> > In general you can use:
> >  region_model::get_rvalue
> > to go from a tree to a symbolic value for what the analyzer
> > "thinks"
> > the value of that tree is at that point along the path.
> > 
> > If it "knows" that it's a specific function pointer, then IIRC this
> > will return a region_svalue where region_svalue::get_pointee ()
> > will
> > (hopefully) point at the function_region representing the memory
> > holding the code of the function.  function_region::get_fndecl
> > should
> > then give you the tree for the specific FUNCTION_DECL, from which
> > you
> > can find the supergraph node etc.
> > 
> > It looks like
> >  region_model::get_fndecl_for_call
> > might already do most of what you need, but it looks like it bails
> > out
> > for the "NULL cgraph_node" case.  Maybe that needs fixing, so that
> > it
> > returns the fndecl for that case?  That already gets used in some
> > places, so maybe try putting a breakpoint on that and see if fixing
> > that gets you further?
> 
> shouldn’t the fn_decl should still have a cgraph_node if the function
> is declared in the program itself ? it should just not have an edge
> representing the call.

That would make sense.  I'd suggest verifying that in the debugger.

> Because I was able to find the super-graph node just with the help of
> the function itself.

Great.


> 
> this is how the function looks "exploded_node::on_edge()" right now.
> 
> File: {$SCR_DIR}/gcc/analyzer/engine.cc
> 1305: bool
> 1306: exploded_node::on_edge (exploded_graph &eg,
> 1307:   const superedge *succ,
> 1308:   program_point *next_point,
> 1309:   program_state *next_state,
> 1310:   uncertainty_t *uncertainty)
> 1311: {
> 1312:   LOG_FUNC (eg.get_logger ());
> 1313: 
> 1314:   if (succ->m_kind == SUPEREDGE_INTRAPROCEDURAL_CALL)
> 1315:   {    
> 1316: const program_point *this_point = &this->get_point();
> 1317: const program_state *this_state = &this->get_state ();
> 1318: const gcall *call = this_point->get_supernode ()-
> >get_final_call ();    
> 1319: 
> 1320: impl_region_model_context ctxt (eg, 
> 1321:   this, 
> 1322:   this_state, 
> 1323:   next_state, 
> 1324:   uncertainty,
> 1325:   this_point->get_stmt());
> 1326: 
> 1327: region_model *model = this_state->m_region_model;
> 1328: tree fn_decl = model->get_fndecl_for_call(call,&ctxt);
> 1329: if(DECL_STRUCT_FUNCTION(fn_decl))
> 1330: {
> 1331:   const supergraph *sg = &eg.get_supergraph();
> 1332:   supernode * sn =  sg->get_node_for_function_entry
> (DECL_STRUCT_FUNCTION(fn_decl));
> 1333:   // create enode and eedge ?
> 1334: }
> 1335:   }
> 1336: 
> 1337:   if (!next_point->on_edge (eg, succ))
> 1338: return false;
> 1339: 
> 1340:   if (!next_state->on_edge (eg, this, succ, uncertainty))
> 1341: return false;
> 1342: 
> 1343:   return true;
> 1344: }

Looks promising.

> 
> for now, it is also detecting calls that already have call_sedge
> connecting them, so I think I also have to filter them out.

Right, I think so too.

Dave

Re: replacing the backwards threader and more

[Aldy Hernandez via Gcc]

I had changed my approach to passing -Wno-free-nonheap-object in the
Makefile. Can you try disabling the Makefile bit and bootstrapping, cause
that was still failing.

Thanks.
Aldy

On Tue, Jun 29, 2021, 00:29 Martin Sebor  wrote:

> On 6/28/21 12:17 AM, Aldy Hernandez wrote:
> >
> >
> > On 6/25/21 7:19 PM, Martin Sebor wrote:
> >> On 6/25/21 10:20 AM, Aldy Hernandez via Gcc wrote:
> >>> Hi folks.
> >>>
> >>> I'm done with benchmarking, testing and cleanups, so I'd like to post
> >>> my patchset for review.  However, before doing so, I'd like to
> >>> address a handful of meta-issues that may affect how I post these
> >>> patches.
> >>>
> >>> Trapping on differences
> >>> ===
> >>>
> >>> Originally I wanted to contribute verification code that would trap
> >>> if the legacy code threaded any edges the new code couldn't (to be
> >>> removed after a week).  However, after having tested on various
> >>> architectures and only running once into a missing thread, I'm
> >>> leaning towards omitting the verification code, since it's fragile,
> >>> time consuming, and quite hacky.
> >>>
> >>> For the record, I have tested on x86-64, aarch64, ppc64 and ppc64le.
> >>> There is only one case, across bootstrap and regression tests where
> >>> the verification code is ever tripped (discussed below).
> >>>
> >>> Performance
> >>> ===
> >>>
> >>> I re-ran benchmarks as per our callgrind suite, and the penalty with
> >>> the current pipeline is 1.55% of overall compilation time.  As is
> >>> being discussed, we should be able to mitigate this significantly by
> >>> removing other threading passes.
> >>>
> >>> Failing testcases
> >>> =
> >>>
> >>> I have yet to run into incorrect code being generated, but I have had
> >>> to tweak a considerable number of tests.  I have verified every
> >>> single discrepancy and documented my changes in the testsuite when it
> >>> merited doing so.  However, there are a couple tests that trigger
> >>> regressions and I'd like to ask for guidance on how to address them.
> >>>
> >>> 1. gcc.c-torture/compile/pr83510.c
> >>>
> >>> I would like to XFAIL this.
> >>>
> >>> What happens here is that thread1 threads a switch statement such
> >>> that the various cases have been split into different independent
> >>> blocks. One of these blocks exposes an arr[i_27] access which is
> >>> later propagated by VRP to be arr[10].  This is an invalid access,
> >>> but the array bounds code doesn't know it is an unreachable path.
> >>
> >> The test has a bunch of loops that iterate over the 10 array elements.
> >> There have been bug reports about loop unrolling causing false positives
> >> -Warray-bounds (e.g., PR 92539, 92110, or 86341) so this could be
> >> the same issue.
> >>
> >>>
> >>> However, it is not until dom2 that we "know" that the value of the
> >>> switch index is such that the path to arr[10] is unreachable.  For
> >>> that matter, it is not until dom3 that we remove the unreachable path.
> >>
> >> If you do XFAIL it can you please isolate a small test case and open
> >> a bug and make it a -Warray-bounds blocker?
> >
> > Will do.
> >
> >>
> >>>
> >>> 2. -Wfree-nonheap-object
> >>>
> >>> This warning is triggered while cleaning up an auto_vec.  I see that
> >>> the va_heap::release() inline is wrapped with a pragma ignore
> >>> "-Wfree-nonheap-object", but this is not sufficient because jump
> >>> threading may alter uses in such a way that may_emit_free_warning()
> >>> will warn on the *inlined* location, thus bypassing the pragma.
> >>>
> >>> I worked around this with a mere:
> >>>
> >>>  > @@ -13839,6 +13839,7 @@ maybe_emit_free_warning (tree exp)
> location_t loc = tree_inlined_location (exp);
>  +  loc = EXPR_LOCATION (exp);
> >>>
> >>> but this causes a ton of Wfree-nonheap* tests to fail.  I think
> >>> someone more knowledgeable should address this (msebor??).
> >>
> >> This sounds like the same problem as PR 98871.  Does the patch below
> >> fix it?
> >> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572515.html
> >> If so, I suggest getting that patch in first to avoid testsuite
> >> failures.  If it doesn't fix it I'll look into it before you commit
> >> your changes.
> >
> > The latest patch in the above thread does not apply.  Do you have a more
> > recent patch?
>
> The first patch in the series applies cleanly for me:
>https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572516.html
> but patch 2/4 does need a few adjustments.  With those and your
> latest changes applied I don't see any -Wfree-nonheap-object test
> failures.
>
> Martin
>
> >
> >>> 3. uninit-pred-9_b.c
> >>>
> >>> The uninit code is getting confused with the threading and the bogus
> >>> warning in line 24 is back.  I looked at the thread, and it is correct.
> >>>
> >>> I'm afraid all these warnings are quite fragile in the presence of
> >>> more aggressive optimizations, and I suspect it will only get worse.
> >>
> >>  From my recent review of