Bug 100537 - [12 Regression] Bootstrap-O3 and bootstrap-debug fail on 32-bit ARM after gcc-12-657-ga076632e274a
Summary: [12 Regression] Bootstrap-O3 and bootstrap-debug fail on 32-bit ARM after gcc...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: go (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: 12.0
Assignee: Ian Lance Taylor
URL:
Keywords:
: 100562 100568 (view as bug list)
Depends on:
Blocks: 100464
  Show dependency treegraph
 
Reported: 2021-05-11 16:39 UTC by Maxim Kuvyrkov
Modified: 2022-02-18 09:29 UTC (History)
8 users (show)

See Also:
Host:
Target: arm-linux-gnueabihf powerpc64le-linux
Build:
Known to work:
Known to fail:
Last reconfirmed: 2021-05-20 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Maxim Kuvyrkov 2021-05-11 16:39:56 UTC
Patch a076632e274abe344ca7648b7c7f299273d4cbe0 appears to have broken bootstrap-O3 and bootstrap-debug at least on 32-bit armhf.

00:33:32 In function ‘syscall.forkExec’:
00:33:32 go1: error: address taken, but ADDRESSABLE bit not set
00:33:32 PHI argument
00:33:32 &go..C479;
00:33:32 for PHI node
00:33:32 err$__object_78 = PHI <err$__object_76(58), &go..C479(59)>
00:33:32 during GIMPLE pass: fre
00:33:32 go1: internal compiler error: verify_ssa failed
00:33:32 0x9c18d7 verify_ssa(bool, bool)
00:33:32 	/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/tree-ssa.c:1214
00:33:32 0x6f8d5b execute_function_todo
00:33:32 	/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/passes.c:2049
00:33:32 0x6f9abf do_per_function
00:33:32 	/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/passes.c:1687
00:33:32 0x6f9abf execute_todo
00:33:32 	/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/passes.c:2096
00:33:32 Please submit a full bug report,
00:33:32 with preprocessed source if appropriate.
00:33:32 Please include the complete backtrace with any bug report.
00:33:32 See <https://gcc.gnu.org/bugs/> for instructions.
00:33:32 Makefile:3001: recipe for target 'syscall.lo' failed
Comment 1 Maxim Kuvyrkov 2021-05-11 16:40:44 UTC
A reduced testcase is coming ...
Comment 2 Maxim Kuvyrkov 2021-05-11 18:45:38 UTC
So far I managed to reproduce this only with armhf Go build, so one needs go1 binary from either a native armhf system or an armhf cross-toolchain to reproduce.

To reproduce on a native system configure GCC with:
../gcc/configure --disable-bootstrap --disable-multilib --with-float=hard --enable-languages=go

To reproduce using a cross-toolchain and Linaro scripts:
git clone https://git.linaro.org/toolchain/abe.git
cd abe
./configure
./abe.sh --target arm-linux-gnueabihf --build all --extraconfigdir config/master
Comment 3 Jiu Fu Guo 2021-05-12 03:01:29 UTC
This issue also occurs on ppc64le when --with-build-config=bootstrap-O3.

As mentioned in PR100513:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100513#c21
Comment 4 Richard Biener 2021-05-12 06:49:10 UTC
So the issue is that the go FE creates go..C479 and it somewhere builds the address of it but fails to set TREE_ADDRESSABLE.  There are only a few
build_fold_addr_expr calls in the FE:

> grep  -r build_fold_addr  .
./go-gcc.cc:  tree t = build_fold_addr_expr_loc(location.gcc_location(), this->t_);
./go-gcc.cc:  tree ret = build_fold_addr_expr_loc(location.gcc_location(), func);
./go-gcc.cc:  tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr);
./go-gcc.cc:          fn = build_fold_addr_expr_loc(location.gcc_location(),
./go-gcc.cc:                          build_fold_addr_expr_loc(location.gcc_location(),

one does mark the object addressable but all the others do not.  So for
those who can reproduce, run the failing compile in a debugger, see
which ADDR_EXPR we complain about and then set a breakpoint in
ggc_internal_alloc conditional on the return value being this ADDR_EXPR.

That should nail one of the above after which you should call
mark_addressable on the object.
Comment 5 Jiu Fu Guo 2021-05-12 09:03:52 UTC
breakpoint at tree-ssa.c:1013 error ("address taken, but ADDRESSABLE bit not set"); 

            if ((VAR_P (base)
                 || TREE_CODE (base) == PARM_DECL
                 || TREE_CODE (base) == RESULT_DECL)
                && !TREE_ADDRESSABLE (base))
              {
B               error ("address taken, but ADDRESSABLE bit not set");
                err = true;
              }

we can see base:
p base
$1 = <var_decl 0x200000a285e0 go..C479>

And break at ggc_internal_alloc, b ggc-page.c:1455 if result ==  0x200000a285e0
we can see the stack:
Unary_expression::do_get_backend (expressions.cc:5322)->Gcc_backend::implicit_variable(go-gcc.cc:29239) -> build_decl->make_node ->...-> ggc_internal_cleared_alloc

Gcc_backend::implicit_variable:
  tree decl = build_decl(BUILTINS_LOCATION, VAR_DECL, ...

Unary_expression::do_get_backend (expressions.cc:5322):
  gogo->backend()->implicit_variable(var_name, "", btype, true, true, false, 0);
where var_name is go..C479
  

And break at build_fold_addr_expr_loc if t == 0x200000a285e0
Gcc_backend::address_expression (go-gcc:1683) --> build_fold_addr_expr_loc
Comment 6 Jiu Fu Guo 2021-05-12 09:16:34 UTC
As Richard mentioned: one does mark the object addressable.
Which is for 'label' (Gcc_backend::label_address).

I'm wondering if all others invoking on build_fold_addr_expr_loc need to mark addressable?
Comment 7 Richard Biener 2021-05-12 09:30:10 UTC
So you can try if the following fixes the bootstrap.

diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc
index 5d9dbb5d068..18673e54c96 100644
--- a/gcc/go/go-gcc.cc
+++ b/gcc/go/go-gcc.cc
@@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* bexpr, Location location)
   if (expr == error_mark_node)
     return this->error_expression();
 
+  mark_addressable (expr);
   tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr);
   return this->make_expression(ret);
 }
Comment 8 Richard Biener 2021-05-12 09:47:29 UTC
*** Bug 100562 has been marked as a duplicate of this bug. ***
Comment 9 Jiu Fu Guo 2021-05-12 11:12:09 UTC
Yes,

diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc
index 5d9dbb5d068..32637a44af1 100644
--- a/gcc/go/go-gcc.cc
+++ b/gcc/go/go-gcc.cc
@@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* bexpr, Location location)
   if (expr == error_mark_node)
     return this->error_expression();

+  TREE_ADDRESSABLE(expr) = 1;
   tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr);
   return this->make_expression(ret);
 }

Could pass bootstrap.
Comment 10 Richard Biener 2021-05-12 15:03:28 UTC
*** Bug 100568 has been marked as a duplicate of this bug. ***
Comment 11 Jiu Fu Guo 2021-05-12 15:11:09 UTC
Had a quick regression test on the patch:
issue4458.go which pass before, but fail on this patch.
Compiling message changed from "error: method expression requires named type or pointer to named type" to "error: method 'foo' is ambiguous"

I'm not sure if this message change is expected or not.

issue4458.go:
package main

type T struct{}

func (T) foo() {}

func main() {
        av := T{}
        pav := &av
        (**T).foo(&pav) // ERROR "no method .*foo|requires named type or pointer to named"
}
Comment 12 Ian Lance Taylor 2021-05-12 21:25:46 UTC
A change to go-gcc.cc should not change any of the error messages emitted by the Go frontend.  It should not change the way that issue4458.go is handled.  Those errors messages are emitted long before any of the code in go-gcc.cc is called.  I'm not sure what is happening.
Comment 13 Jiu Fu Guo 2021-05-13 02:19:09 UTC
(In reply to Ian Lance Taylor from comment #12)
> A change to go-gcc.cc should not change any of the error messages emitted by
> the Go frontend.  It should not change the way that issue4458.go is handled.
> Those errors messages are emitted long before any of the code in go-gcc.cc
> is called.  I'm not sure what is happening.

It is interesting,  I rerun for trunk (without the patch), the message is "error: type has no method 'foo'"
With the patch, the message is "error: method 'foo' is ambiguous"

At expressions.cc:14655
          if (!is_ambiguous)
            go_error_at(location, "type has no method %<%s%>",
                        Gogo::message_name(name).c_str());
          else
            go_error_at(location, "method %<%s%> is ambiguous",
			Gogo::message_name(name).c_str());

is_ambiguous:
  if (nt != NULL)
    method = nt->method_function(name, &is_ambiguous);
  else if (st != NULL)
    method = st->method_function(name, &is_ambiguous);
Comment 14 Jiu Fu Guo 2021-05-13 07:00:37 UTC
Update/correct info:
If bootstrap-O3, the message is "error: method 'foo' is ambiguous".
It is "error: type has no method 'foo'".
Comment 15 Jiu Fu Guo 2021-05-13 07:02:11 UTC
(In reply to Jiu Fu Guo from comment #9)
> Yes,
> 
> diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc
> index 5d9dbb5d068..32637a44af1 100644
> --- a/gcc/go/go-gcc.cc
> +++ b/gcc/go/go-gcc.cc
> @@ -1680,6 +1680,7 @@ Gcc_backend::address_expression(Bexpression* bexpr,
> Location location)
>    if (expr == error_mark_node)
>      return this->error_expression();
> 
> +  TREE_ADDRESSABLE(expr) = 1;
>    tree ret = build_fold_addr_expr_loc(location.gcc_location(), expr);
>    return this->make_expression(ret);
>  }
> 
> Could pass bootstrap.

So, this patch would pass bootstrap and regtest.
Comment 16 Ian Lance Taylor 2021-05-24 15:55:01 UTC
I have a patch for this.
Comment 17 GCC Commits 2021-05-24 20:25:07 UTC
The master branch has been updated by Ian Lance Taylor <ian@gcc.gnu.org>:

https://gcc.gnu.org/g:358832c46a378e5a0b8a2fa3c2739125e3e680c7

commit r12-1022-g358832c46a378e5a0b8a2fa3c2739125e3e680c7
Author: Ian Lance Taylor <iant@golang.org>
Date:   Sat May 22 19:19:13 2021 -0700

    compiler: mark global variables whose address is taken
    
    To implement this, change the backend to use flag bits for variables.
    
    Fixes https://gcc.gnu.org/PR100537
    
            PR go/100537
            * go-gcc.cc (class Gcc_backend): Update methods that create
            variables to take a flags parameter.
    
    Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/322129
Comment 18 Ian Lance Taylor 2021-05-24 20:58:43 UTC
Should be fixed.
Comment 19 Richard Biener 2022-02-17 10:23:22 UTC
Ian, the PR100464 fix depends on this but when trying to cherry-pick the Go fix to the gcc-11 branch I get a conflict in gcc/go/gofrontend/MERGE:

<<<<<<< HEAD
9782e85bef1c16c72a4980856d921cea104b129c
=======
5a801b15699cced5203af5c7339b375cd55ecbac
>>>>>>> 358832c46a3 (compiler: mark global variables whose address is taken)

The first line of this file holds the git revision number of the last
merge done from the gofrontend repository.

How should I handle backporting of such fixes?
Comment 20 Ian Lance Taylor 2022-02-17 16:18:14 UTC
There's no perfect way to handle the MERGE file on the release branches.  What I usually do is to resolve the patch by replacing the existing revision number with the new one.  Thanks.
Comment 21 GCC Commits 2022-02-18 09:29:17 UTC
The releases/gcc-11 branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:8a1e92ff45e8e254fb557d20dcfa54a88d354329

commit r11-9592-g8a1e92ff45e8e254fb557d20dcfa54a88d354329
Author: Ian Lance Taylor <iant@golang.org>
Date:   Sat May 22 19:19:13 2021 -0700

    compiler: mark global variables whose address is taken
    
    To implement this, change the backend to use flag bits for variables.
    
    Fixes https://gcc.gnu.org/PR100537
    
            PR go/100537
            * go-gcc.cc (class Gcc_backend): Update methods that create
            variables to take a flags parameter.
    
    Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/322129
    (cherry picked from commit 358832c46a378e5a0b8a2fa3c2739125e3e680c7)