Surprising CFG construction with goto from then to else

[Jørgen Kvalsvik]

Hello,

I played some more with odd programs and the effect on control flow 
graph construction (as a part of condition coverage support [1]) and 
came across this:


int fn (int a, int b, int c) {
int x = 0;
if (a && b) {
if (c) {
goto a_;
} else {
x = a;
}
} else {
a_:
x = (a - b);
}

return x;
}

Run through gcov --conditions I get:

4:5:if (a && b) {
condition outcomes covered 2/2
condition outcomes covered 2/2
2:6:if (c) {
condition outcomes covered 2/2

Which is clearly not correct. So I started digging into why and dump the 
CFG as the coverage profiling sees it https://i.imgur.com/d0q72rA.png 
[2]. I apologize for the labeling, but A2 = a, A3 = b, A5 = c and A9 the 
else block. The problem, which is what confuses the algorithm, is that a 
and b don't share A9 as a successor (on false) as I would expect.


If I add some operation before the label the problem disappears and a 
and b share false-destination again https://i.imgur.com/PSrfaLC.png [3].


} else {
x++;
a_:
x = (a - b);
}

4:5:if (a && b) {
condition outcomes covered 4/4
2:6:if (c) {
condition outcomes covered 2/2


When dumping the cfg in the former case with -fdump-tree-cfg-graph I get 
a CFG without the split destinations in a and b 
https://i.imgur.com/05MCjzp.png [3]. I would assume from this that the 
graph dump happens after _more_ CFG transformations than the branch 
profiling.


So my questions are:

1. Is the control flow graph expected to be constructed as such where a 
and b don't share outcome, or is it to be considered a bug?
2. If yes, would it be problematic to push the branch coverage and 
condition profiling to a later stage where the cfg has been fixed?


Thanks,
Jørgen

[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598165.html

[2] dot file:

digraph {
A0 -> A2
A2 -> A3
A2 -> A8
A3 -> A5
A3 -> A4
A8 -> A9
A9 -> A10
A10 -> A11
A11 -> A1
A5 -> A6
A5 -> A7
A4 -> A9
A6 -> A9
A7 -> A10
}

[3] dot file:

digraph {
A0 -> A2
A2 -> A3
A2 -> A7
A3 -> A4
A3 -> A7
A7 -> A8
A8 -> A9
A9 -> A10
A10 -> A1
A4 -> A5
A4 -> A6
A5 -> A8
A6 -> A9
}

[3] dot file:


overlap=false;
subgraph "cluster_fn" {
style="dashed";
color="black";
label="fn ()";
fn_0_basic_block_0 
[shape=Mdiamond,style=filled,fillcolor=white,label="ENTRY"];


fn_0_basic_block_1 
[shape=Mdiamond,style=filled,fillcolor=white,label="EXIT"];


fn_0_basic_block_2 
[shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\

|x\ =\ 0;\l\
|if\ (a\ !=\ 0)\l\
\ \ goto\ \;\ [INV]\l\
else\l\
\ \ goto\ \;\ [INV]\l\
}"];

fn_0_basic_block_3 
[shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\

|if\ (b\ !=\ 0)\l\
\ \ goto\ \;\ [INV]\l\
else\l\
\ \ goto\ \;\ [INV]\l\
}"];

fn_0_basic_block_4 
[shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\

|if\ (c\ !=\ 0)\l\
\ \ goto\ \;\ [INV]\l\
else\l\
\ \ goto\ \;\ [INV]\l\
}"];

fn_0_basic_block_5 
[shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\

|//\ predicted\ unlikely\ by\ goto\ predictor.\l\
goto\ \;\ [INV]\l\
}"];

fn_0_basic_block_6 
[shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\

|x\ =\ a;\l\
goto\ \;\ [INV]\l\
}"];

fn_0_basic_block_7 
[shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\

|a_:\l\
|x\ =\ a\ -\ b;\l\
}"];

fn_0_basic_block_8 
[shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\

|D.2398\ =\ x;\l\
}"];

fn_0_basic_block_9 
[shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\

|\:\l\
|return\ D.2398;\l\
}"];

fn_0_basic_block_0:s -> fn_0_basic_block_2:n 
[style="solid,bold",color=black,weight=100,constraint=true];
fn_0_basic_block_2:s -> fn_0_basic_block_3:n 
[style="solid,bold",color=forestgreen,weight=10,constraint=true];
fn_0_basic_block_2:s -> fn_0_basic_block_7:n 
[style="solid,bold",color=darkorange,weight=10,constraint=true];
fn_0_basic_block_3:s -> fn_0_basic_block_4:n 
[style="solid,bold",color=forestgreen,weight=10,constraint=true];
fn_0_basic_block_3:s -> fn_0_basic_block_7:n 
[style="solid,bold",color=darkorange,weight=10,constraint=true];
fn_0_basic_block_4:s -> fn_0_basic_block_5:n 
[style="solid,bold",color=forestgreen,weight=10,constraint=true];
fn_0_basic_block_4:s -> fn_0_basic_block_6:n 
[style="solid,bold",color=darkorange,weight=10,constraint=true];
fn_0_basic_block_5:s -> fn_0_basic_block_7:n 
[style="solid,bold",color=black,weight=100,constraint=true];
fn_0_basic_block_6:s -> fn_0_basic_block_8:n 
[style="solid,bold",color=black,weight=100,constraint=true];
fn_0_basic_block_7:s -> fn_0_basic_block_8:n 
[style="solid,bold",color=black,weight=100,constraint=true];

Re: Surprising CFG construction with goto from then to else

[Richard Biener via Gcc]

On Fri, Sep 2, 2022 at 11:50 AM Jørgen Kvalsvik  wrote:
>
>
> Hello,
>
> I played some more with odd programs and the effect on control flow
> graph construction (as a part of condition coverage support [1]) and
> came across this:
>
> int fn (int a, int b, int c) {
>  int x = 0;
>  if (a && b) {
>  if (c) {
>  goto a_;
>  } else {
>  x = a;
>  }
>  } else {
> a_:
>  x = (a - b);
>  }
>
>  return x;
> }
>
> Run through gcov --conditions I get:
>
>  4:5:if (a && b) {
> condition outcomes covered 2/2
> condition outcomes covered 2/2
>  2:6:if (c) {
> condition outcomes covered 2/2
>
> Which is clearly not correct. So I started digging into why and dump the
> CFG as the coverage profiling sees it https://i.imgur.com/d0q72rA.png
> [2]. I apologize for the labeling, but A2 = a, A3 = b, A5 = c and A9 the
> else block. The problem, which is what confuses the algorithm, is that a
> and b don't share A9 as a successor (on false) as I would expect.
>
> If I add some operation before the label the problem disappears and a
> and b share false-destination again https://i.imgur.com/PSrfaLC.png [3].
>
>  } else {
>  x++;
> a_:
>  x = (a - b);
>  }
>
>  4:5:if (a && b) {
> condition outcomes covered 4/4
>  2:6:if (c) {
> condition outcomes covered 2/2
>
>
> When dumping the cfg in the former case with -fdump-tree-cfg-graph I get
> a CFG without the split destinations in a and b
> https://i.imgur.com/05MCjzp.png [3]. I would assume from this that the
> graph dump happens after _more_ CFG transformations than the branch
> profiling.
>
> So my questions are:
>
> 1. Is the control flow graph expected to be constructed as such where a
> and b don't share outcome, or is it to be considered a bug?
> 2. If yes, would it be problematic to push the branch coverage and
> condition profiling to a later stage where the cfg has been fixed?

I would say you should only see more nodes merged.  It's a bit hard to follow
what you say with the namings - I usually run cc1 in gdb, breaking at
execute_build_cfg where you can do, after build_gimple_cfg finished
(and before cleanup_tree_cfg ()) do a 'dot-fn' in gdb which produces a nice
picture of the CFG and code with graphviz.

It looks like I would have expected, in particular we do not force a
new basic-block to be generated for a_: after the D.1991: artificial
label we have for the else.  That might be premature optimization
for your case (but the cleanup_tree_cfg () would immediately do
that as well).

Richard.

> Thanks,
> Jørgen
>
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598165.html
>
> [2] dot file:
>
> digraph {
>  A0 -> A2
>  A2 -> A3
>  A2 -> A8
>  A3 -> A5
>  A3 -> A4
>  A8 -> A9
>  A9 -> A10
>  A10 -> A11
>  A11 -> A1
>  A5 -> A6
>  A5 -> A7
>  A4 -> A9
>  A6 -> A9
>  A7 -> A10
> }
>
> [3] dot file:
>
> digraph {
>  A0 -> A2
>  A2 -> A3
>  A2 -> A7
>  A3 -> A4
>  A3 -> A7
>  A7 -> A8
>  A8 -> A9
>  A9 -> A10
>  A10 -> A1
>  A4 -> A5
>  A4 -> A6
>  A5 -> A8
>  A6 -> A9
> }
>
> [3] dot file:
>
>
> overlap=false;
> subgraph "cluster_fn" {
>  style="dashed";
>  color="black";
>  label="fn ()";
>  fn_0_basic_block_0
> [shape=Mdiamond,style=filled,fillcolor=white,label="ENTRY"];
>
>  fn_0_basic_block_1
> [shape=Mdiamond,style=filled,fillcolor=white,label="EXIT"];
>
>  fn_0_basic_block_2
> [shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\
> |x\ =\ 0;\l\
> |if\ (a\ !=\ 0)\l\
> \ \ goto\ \;\ [INV]\l\
> else\l\
> \ \ goto\ \;\ [INV]\l\
> }"];
>
>  fn_0_basic_block_3
> [shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\
> |if\ (b\ !=\ 0)\l\
> \ \ goto\ \;\ [INV]\l\
> else\l\
> \ \ goto\ \;\ [INV]\l\
> }"];
>
>  fn_0_basic_block_4
> [shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\
> |if\ (c\ !=\ 0)\l\
> \ \ goto\ \;\ [INV]\l\
> else\l\
> \ \ goto\ \;\ [INV]\l\
> }"];
>
>  fn_0_basic_block_5
> [shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\
> |//\ predicted\ unlikely\ by\ goto\ predictor.\l\
> goto\ \;\ [INV]\l\
> }"];
>
>  fn_0_basic_block_6
> [shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\
> |x\ =\ a;\l\
> goto\ \;\ [INV]\l\
> }"];
>
>  fn_0_basic_block_7
> [shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\
> |a_:\l\
> |x\ =\ a\ -\ b;\l\
> }"];
>
>  fn_0_basic_block_8
> [shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\
> |D.2398\ =\ x;\l\
> }"];
>
>  fn_0_basic_block_9
> [shape=record,style=filled,fillcolor=lightgrey,label="{\:\l\
> |\:\l\
> |return\ D.2398;\l\
> }"];
>
>  fn_0_basic_block_0:s -> fn_0_basic_block_2:n
> [style="solid,bold",color=black,weight=100,constraint=true];
>  fn_0_basic_block_2:s -> 

Re: [PATCH 0/3] picolibc: Add picolibc linking help

[Richard Sandiford via Gcc]

Keith Packard via Gcc-patches  writes:
> Picolibc is a C library for embedded systems based on code from newlib
> and avr libc. To connect some system-dependent picolibc functions
> (like stdio) to an underlying platform, the platform may provide an OS
> library.
>
> This OS library must follow the C library in the link command line. In
> current picolibc, that is done by providing an alternate .specs file
> which can rewrite the *lib spec to insert the OS library in the right
> spot.
>
> This patch series adds the ability to specify the OS library on the
> gcc command line when GCC is configured to us picolibc as the default
> C library, and then hooks that up for arm, nds32, riscv and sh targets.

Not really my area, but the approach LGTM FWIW.  Main question/points:

- In:

  +case "${with_default_libc}" in
  +glibc)
  +default_libc=LIBC_GLIBC
  +;;

  should there be a default case that raises an error for unrecognised
  libcs?  Command-line checking for configure isn't very tight, but we
  do raise similar errors for things like invalid --enable-threads values.

- I'm not sure either way about adding LIBC_NEWLIB.  On the one hand
  it makes sense for completeness, but on the other it's write-only.
  Adding it means that --with-default-libc=newlib toolchains have a
  different macro configuration from a default toolchain even in cases
  where newlib is the default.

  On balance I think it would be better to accept
  --with-default-libc=newlib but set default_libc to the empty string.

- Should we raise an error for toolchains that don't support the given
  C library?  It feels like we should, but I realise that could be
  difficult to do.

- Very minor, but in lines like:

  +#if defined(DEFAULT_LIBC) && defined(LIBC_PICOLIBC) && DEFAULT_LIBC == 
LIBC_PICOLIBC

  is LIBC_PICOLIB ever undefined?  It looks like config.gcc provides
  an unconditional definition.  If it is always defined then:

  #if DEFAULT_LIBC == LIBC_PICOLIB

  would be clearer.

Thanks,
Richard

>
> Keith Packard (3):
>   Allow default libc to be specified to configure
>   Add newlib and picolibc as default C library choices
>   Add '--oslib=' option when default C library is picolibc
>
>  gcc/config.gcc| 56 ---
>  gcc/config/arm/elf.h  |  5 
>  gcc/config/nds32/elf.h|  4 +++
>  gcc/config/picolibc.opt   | 26 ++
>  gcc/config/riscv/elf.h|  4 +++
>  gcc/config/sh/embed-elf.h |  5 
>  gcc/configure.ac  |  4 +++
>  7 files changed, 95 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/config/picolibc.opt

Setting test suite not to try debug output cases

[Koning, Paul via Gcc]

Given that pdp11 no longer supports debug output, I get a lot more test 
failures, like this:

spawn -ignore SIGHUP /Users/pkoning/Documents/svn/build/pdp/gcc/xgcc 
-B/Users/pkoning/Documents/svn/build/pdp/gcc/ -mlra -fdiagnostics-plain-output 
-Og -g -w -c -o 2105-1.o 
/Users/pkoning/Documents/svn/gcc/gcc/testsuite/gcc.c-torture/compile/2105-1.c
xgcc: warning: target system does not support debug output
cc1: warning: target system does not support debug output
FAIL: gcc.c-torture/compile/2105-1.c   -Og -g  (test for excess errors)

I assume there is some way in the test suite machinery to globally skip all 
"debug output" cases.  How would I do that?

paul

Re: GNU Tools Cauldron 2022

[Thomas Schwinge]

Hi!

(Currently still on parental leave, but I just had to...)  ;-P

On 2022-05-15T01:02:07+0200, Jan Hubicka via Gcc  wrote:
> We are pleased to invite you all to the next GNU Tools Cauldron,
> taking place in [Prague] on September 16-18, 2022.  We are looking forward
> to meet you again after three years!
>
> As for the previous instances, we have setup a wiki page for
> details:
>
>  https://gcc.gnu.org/wiki/cauldron2022  
> 

Pushed to wwwdocs commit b465e8f1f72a0718aebcf483a78b68c3e68ead72
"GNU Tools Cauldron 2022", see attached.


See you in less than two weeks!

Grüße
 Thomas


>From b465e8f1f72a0718aebcf483a78b68c3e68ead72 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Fri, 2 Sep 2022 21:39:18 +0200
Subject: [PATCH] GNU Tools Cauldron 2022

---
 htdocs/index.html | 4 
 1 file changed, 4 insertions(+)

diff --git a/htdocs/index.html b/htdocs/index.html
index 3bec379e..c2982a74 100644
--- a/htdocs/index.html
+++ b/htdocs/index.html
@@ -55,6 +55,10 @@ mission statement.
 News
 
 
+https://gcc.gnu.org/wiki/cauldron2022";>GNU Tools Cauldron 2022
+[2022-09-02]
+Prague, Czech Republic and online, September 16-18 2022
+
 GCC 12.2 released
 [2022-08-19]
 
-- 
2.35.1

Re: [PATCH] Various pages: SYNOPSIS: Use VLA syntax in function parameters

[Alejandro Colomar via Gcc]

Hi JeanHeyd,

Subject: Re: [PATCH] Various pages: SYNOPSIS: Use VLA syntax in 
function parameters

Date: Fri, 2 Sep 2022 16:56:00 -0400
From: JeanHeyd Meneide 
To: Alejandro Colomar 
CC: Ingo Schwarze , linux-...@vger.kernel.org



Hi Alejandro and Ingo,

   Just chiming in from a Standards perspective, here. We discussed, 
briefly, a way to allow Variable-Length function parameter declarations 
like the ones shown in this thread (e.g., char *getcwd(char buf[size], 
size_t size );).


   In GCC, there is a GNU extension that allows explicitly 
forward-declaring the prototype. Using the above example, it would look 
like so:


I added the GCC list to the thread, so that they can intervene if they 
consider it necessary.




char *getcwd(size_t size; char buf[size], size_t size);


I read about that, although I don't like it very much, and never used it.



(Live Example [1])

(Note the `;` after the first "size" declaration). This was brought 
before the Committee to vote on for C23 in the form of N2780 [2], around 
the January 2022 timeframe. The paper did not pass, and it was seen as a 
"failed extension". After the vote on that failed, we talked about other 
ways of allowing places whether there was some appetite to allow 
"forward parsing" for this sort of case. That is, could we simply allow:


char *getcwd(char buf[size], size_t size);

to work as expected. The vote for this did not gain full consensus 
either, but there were a lot of abstentions [3]. While I personally 
voted in favor of allowing such for C, there was distinct worry that 
this would produce issues for weaker C implementations that did not want 
to commit to delayed parsing or forward parsing of the entirety of the 
argument list before resolving types. There are enough abstentions 
during voting that a working implementation with a writeup of complexity 
would sway the Committee one way or the other.


I like that this got less hate than the GNU extension.  It's nicer to my 
eyes.




This is not to dissuade Alejandro's position, or to bolster Ingo's 
point; I'm mostly just reporting the Committee's response here. This is 
an unsolved problem for the Committee, and also a larger holdover from 
the removal of K&R declarations from C23, which COULD solve this problem:


// decl
char *getcwd();

// impl
char* getcwd(buf, size)
char buf[size];
   size_t size;
{
   /* impl here */
}


I won't miss them ;)

My regex-based parser[1] that finds declarations and definitions in C 
code bases goes nuts with K&R functions.  They are dead for good :)


[1]: 



   There is room for innovation here, or perhaps bolstering of the 
GCC original extension. As it stands right now, compilers only very 
recently started taking Variably-Modified Type parameters and Static 
Extent parameters seriously after carefully separating them out of 
Variable-Length Arrays, warning where they can when static or other 
array parameters do not match buffer lengths and so-on.


   Not just to the folks in this thread, but to the broader 
community for anyone who is paying attention: WG14 would actively like 
to solve this problem. If someone can:

- prove out a way to do delayed parsing that is not implementation-costly,
- revive the considered-dead GCC extension, or
- provide a 3rd or 4th way to support the goals,

I am certain WG14 would look favorably upon such a thing eventually, 
brought before the Committee in inclusion for C2y/C3a.


   Whether or not you feel like the manpages are the best place to 
start that, I'll leave up to you!


I'll try to defend the reasons to start this in the man-pages.

This feature is mostly for documentation purposes, not being meaningful 
for code at all (for some meaning of meaningful), since it won't change 
the function definition in any way, nor the calls to it.  At least not 
by itself; static analysis may get some benefits, though.


Also, new code can be designed from the beginning so that sizes go 
before their corresponding arrays, so that new code won't typically be 
affected by the lack of this feature in the language.


This leaves us with legacy code, especially libc, which just works, and 
doesn't have any urgent needs to change their prototypes in this regard 
(they could, to improve static analysis, but not what we'd call urgent).


And since most people don't go around reading libc headers searching for 
function declarations (especially since there are manual pages that show 
them nicely), it's not like the documentation of the code depends on how 
the function is _actually_ declared in code (that's why I also defended 
documenting restrict even if glibc wouldn't have cared to declare it), 
but it depends basically on what the manual pages say about the 
function.  If the manual pages say a function gets 'restrict' params, it 
means it gets 'restrict' params, no matter what the code says, and if it 
doesn't, 

gcc-11-20220902 is now available

[GCC Administrator via Gcc]

Snapshot gcc-11-20220902 is now available on
  https://gcc.gnu.org/pub/gcc/snapshots/11-20220902/
and on various mirrors, see http://gcc.gnu.org/mirrors.html for details.

This snapshot has been generated from the GCC 11 git branch
with the following options: git://gcc.gnu.org/git/gcc.git branch 
releases/gcc-11 revision 50982aa1145fbdb315162349833412639aa8bc4c

You'll find:

 gcc-11-20220902.tar.xz   Complete GCC

  SHA256=3d160d00d3b64697653e0f426cabd90fd5b167793d6cfa6cff9503ab98b3c1cf
  SHA1=44b61b919e9d0128ba0f828ebb29aa9b9d762262

Diffs from 11-20220826 are available in the diffs/ subdirectory.

When a particular snapshot is ready for public consumption the LATEST-11
link is updated and a message is sent to the gcc list.  Please do not use
a snapshot before it has been announced that way.

Re: [PATCH 0/3] picolibc: Add picolibc linking help

[Keith Packard via Gcc]

Richard Sandiford  writes:

Thanks much for reviewing this series. I really appreciate it.

>   should there be a default case that raises an error for unrecognised
>   libcs?  Command-line checking for configure isn't very tight, but we
>   do raise similar errors for things like invalid --enable-threads
>   values.

Good thinking. It's way to easy to introduce a typo somewhere and have
it get missed.

>   On balance I think it would be better to accept
>   --with-default-libc=newlib but set default_libc to the empty string.

That also makes good sense -- leave existing configurations unchanged.

> - Should we raise an error for toolchains that don't support the given
>   C library?  It feels like we should, but I realise that could be
>   difficult to do.

That would be nice, but it also feels like making it reliable enough to
be useful would be difficult to maintain in the future, even if we did
manage to make it mostly work today.

> - Very minor, but in lines like:
>
>   +#if defined(DEFAULT_LIBC) && defined(LIBC_PICOLIBC) && DEFAULT_LIBC == 
> LIBC_PICOLIBC
>
>   is LIBC_PICOLIB ever undefined?  It looks like config.gcc provides
>   an unconditional definition.  If it is always defined then:
>
>   #if DEFAULT_LIBC == LIBC_PICOLIB
>
>   would be clearer.

Agreed. Thanks again for taking a look; I'll send a new series with
these issues fixed shortly.

-- 
-keith


signature.asc
Description: PGP signature