Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

error: storing the address of local variable 'stackOut' in '*stackOut.0_1' [-Werror=dangling-pointer=] #256

Open
topazus opened this issue Jul 12, 2023 · 13 comments · May be fixed by #257
Open
Assignees
Labels
build Problems with builds and build automation essential Will cause failure to meet customer requirements. Assign resources.

Comments

@topazus
Copy link

topazus commented Jul 12, 2023

I try to compile mps 1.118.0 with GCC 13 on Fedora 39, but I occured the build error.

lii6gc: lii6gc/cool/ss.o
ss.c: In function 'StackHot':
ss.c:38:13: error: storing the address of local variable 'stackOut' in '*stackOut.0_1' [-Werror=dangling-pointer=]
   38 |   *stackOut = &stackOut;
      |   ~~~~~~~~~~^~~~~~~~~~~
ss.c:36:22: note: 'stackOut' declared here
   36 | void StackHot(void **stackOut)
      |               ~~~~~~~^~~~~~~~
ss.c:36:22: note: 'stackOut' declared here
cc1: all warnings being treated as errors
gmake[3]: *** [comm.gmk:628: lii6gc/cool/ss.o] Error 1
gmake[3]: Leaving directory '/home/ruby/rpmbuild/BUILD/mps-release-1.118.0/code'
gmake[2]: *** [comm.gmk:398: target] Error 2
gmake[1]: *** [comm.gmk:370: mps.a] Error 2
gmake[1]: *** Waiting for unfinished jobs....
@rptb1 rptb1 added the build Problems with builds and build automation label Jul 12, 2023
@thejayps
Copy link
Contributor

Many thanks for your report. Can you tell us anything more about how what you're working on uses the MPS?

We currently build and test the MPS on Ubuntu 22, which is currently on GCC 11.3.0. We'll investigate the GCC 13 build.

If you are not modifying the MPS itself, you can build it for your project by following 2.3. Compiling the MPS for your project https://memory-pool-system.readthedocs.io/en/version-1.118/guide/build.html#compiling-the-mps-for-your-project. This will not produce warnings as errors.

If you're developing the MPS, you can work around this problem locally by modifying code/gc.gmk to suppress this warning, by adding something like -Wno-error=dangling-pointer to CFLAGSCOMPILER.

If you are integrating the MPS with Ruby (or have done so), we'd very much like to hear about it.

@rptb1
Copy link
Member

rptb1 commented Jul 12, 2023

I run Ubuntu 13 minimal (from ubuntu-23.04-desktop-amd64.iso) in a VM and installed the default gcc (gcc-12.2.0-1ubuntu1) and gcc-13 (gcc-13-20230320-1ubuntu1) with apt:

sudo apt-get install git gcc make
git clone https://github.com/Ravenbrook/mps.git
cd mps
./configure && make
sudo apt-get install gcc-13
CC=gcc-13 ./configure && make

Both fail with the same warning error:

lii6gc: lii6gc/hot/mps.o
In file included from /usr/include/string.h:535,
                 from mpsliban.c:33,
                 from mps.c:98:
In function \u2018memcpy\u2019,
    inlined from \u2018mps_lib_memcpy\u2019 at mpsliban.c:105:10,
    inlined from \u2018VMArenaCreate\u2019 at arenavm.c:683:9:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: \u2018vmParams\u2019 may be used uninitialized [-Werror=maybe-uninitialized]
   29 |   return __builtin___memcpy_chk (__dest, __src, __len,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   30 |                                  __glibc_objsize0 (__dest));
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from mps.c:35:
arenavm.c: In function \u2018VMArenaCreate\u2019:
arenavm.c:603:8: note: \u2018vmParams\u2019 declared here
  603 |   char vmParams[VMParamSize];
      |        ^~~~~~~~
cc1: all warnings being treated as errors

This isn't the same as observed above, but it indicated we have some work to do to ensure we build cleanly under GCC 12 and 13 both.

@waywardmonkeys
Copy link
Contributor

In GitHub Actions, ubuntu-latest should have gcc-12 / g++-12 available and you can test multiple versions using a matrix. (It should have gcc 9, 10, 11, and 12 as well as clang 12, 13, 14.)

@topazus
Copy link
Author

topazus commented Jul 12, 2023

Both fail with the same warning error:

lii6gc: lii6gc/hot/mps.o
In file included from /usr/include/string.h:535,
                 from mpsliban.c:33,
                 from mps.c:98:
In function \u2018memcpy\u2019,
    inlined from \u2018mps_lib_memcpy\u2019 at mpsliban.c:105:10,
    inlined from \u2018VMArenaCreate\u2019 at arenavm.c:683:9:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: \u2018vmParams\u2019 may be used uninitialized [-Werror=maybe-uninitialized]
   29 |   return __builtin___memcpy_chk (__dest, __src, __len,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   30 |                                  __glibc_objsize0 (__dest));
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from mps.c:35:
arenavm.c: In function \u2018VMArenaCreate\u2019:
arenavm.c:603:8: note: \u2018vmParams\u2019 declared here
  603 |   char vmParams[VMParamSize];
      |        ^~~~~~~~
cc1: all warnings being treated as errors

This isn't the same as observed above, but it indicated we have some work to do to ensure we build cleanly under GCC 12 and 13 both.

diff --git a/code/ss.c b/code/ss.c
index ce08810..2dea149 100644
--- a/code/ss.c
+++ b/code/ss.c
@@ -19,6 +19,7 @@
  * stackCold).
  */
 
+#include <stdlib.h>
 #include "mpm.h"
 
 SRCID(ss, "$Id$");
@@ -35,7 +36,8 @@ SRCID(ss, "$Id$");
 ATTRIBUTE_NOINLINE
 void StackHot(void **stackOut)
 {
-  *stackOut = &stackOut;
+  void *newStackOut = malloc(sizeof(void *));
+  *stackOut = newStackOut;
 }

After applying the above patch (not sure it is correct), I retried to build, also failed with the similar error.

lii6gc: lii6gc/hot/abqtest.o
lii6gc: lii6gc/hot/mps.o
In file included from mps.c:98:
In function ‘mps_lib_memcpy’,
    inlined from ‘VMArenaCreate’ at arenavm.c:683:9:
mpsliban.c:105:10: error: ‘vmParams’ may be used uninitialized [-Werror=maybe-uninitialized]
  105 |   return memcpy(s1, s2, n);
      |          ^~~~~~~~~~~~~~~~~
In file included from mps.c:35:
arenavm.c: In function ‘VMArenaCreate’:
arenavm.c:603:8: note: ‘vmParams’ declared here
  603 |   char vmParams[VMParamSize];
      |        ^~~~~~~~
cc1: all warnings being treated as errors

@rptb1
Copy link
Member

rptb1 commented Jul 12, 2023

In GitHub Actions, ubuntu-latest should have gcc-12 / g++-12 available and you can test multiple versions using a matrix. (It should have gcc 9, 10, 11, and 12 as well as clang 12, 13, 14.)

So I think we can extend the matrix here

matrix:
os: [ubuntu-latest, macos-latest]
compiler: [clang, gcc]
to get CI coverage and fix up these warnings.

@rptb1
Copy link
Member

rptb1 commented Jul 12, 2023

After applying the above patch (not sure it is correct), I retried to build, also failed with the similar error.

Nice try! :) Just for the record, that patch will break the stack scanning in the MPS. The purpose is to get an address at the "hot" end of the stack by taking the address of an argument (spilled local variable). It's a weird thing to do and I'm not surprised the compiler has got around to noticing it. It's an attempt to avoid the need for assembler code to get the stack pointer.

@rptb1 rptb1 linked a pull request Jul 23, 2023 that will close this issue
@rptb1
Copy link
Member

rptb1 commented Jul 23, 2023

In GitHub Actions, ubuntu-latest should have gcc-12 / g++-12 available and you can test multiple versions using a matrix. (It should have gcc 9, 10, 11, and 12 as well as clang 12, 13, 14.)

Unfortunately, the GCC 12 available does not provoke this warning.

On ubuntu-latest (see https://github.com/Ravenbrook/mps/actions/runs/5635102961/job/15265698554?pr=257 ):

Run gcc-12 --version
gcc-12 (Ubuntu 12.1.0-2ubuntu1~22.04) 12.1.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Whereas on ubuntu-23.04-desktop-amd64.iso (via https://releases.ubuntu.com/23.04/ubuntu-23.04-desktop-amd64.iso.torrent ):

ubuntu@ubuntu:~$ gcc-12 --version
gcc-12 (Ubuntu 12.2.0-17ubuntu1) 12.2.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

ubuntu@ubuntu:~$ gcc-13 --version
gcc-13 (Ubuntu 13-20230320-1ubuntu1) 13.0.1 20230320 (experimental) [master r13-6759-g5194ad1958c]
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

So I think this warning was introduced in GCC 12.2.

@rptb1
Copy link
Member

rptb1 commented Jul 23, 2023

@topazus 8e117ae (in #257 ) resolves warning on GCC 12.2.0 and GCC 13.0.1, passing make test. I am not able to reproduce the error in your original post on Ubuntu. Can you provide the exact GCC version that produces the error, or a download link for an OS image I can run in a VM to reproduce the error? Thanks.

@topazus
Copy link
Author

topazus commented Jul 23, 2023

Can you provide the exact GCC version that produces the error, or a download link for an OS image I can run in a VM to reproduce the error? Thanks.

I opened #258 to add Fedora CI which will reproduce the error. The build log of github workflow: https://github.com/topazus/mps/actions/runs/5636841949/job/15269323018

If you're developing the MPS, you can work around this problem locally by modifying code/gc.gmk to suppress this warning, by adding something like -Wno-error=dangling-pointer to CFLAGSCOMPILER.

For the time being, we may add the -Wno-error=dangling-pointer to compiler flag, which can convert this error reporting to warning for further compiling.

@rptb1
Copy link
Member

rptb1 commented Jul 25, 2023

I opened #258 to add Fedora CI which will reproduce the error. The build log of github workflow: https://github.com/topazus/mps/actions/runs/5636841949/job/15269323018

Thank you. We'll consider integrating this into our usual CI in order to get early warnings of... warnings.

@rptb1
Copy link
Member

rptb1 commented Jul 25, 2023

I found the Fedora nightly compose finder which provided Fedora-Everything-netinst-x86_64-Rawhide-20230724.n.0.iso from which I was able to create a VM. The GCC version is 13.1.1. I was able to reproduce the error in the original post above. So I suspect that the warning was introduced in GCC 13.1, since it is not present in 13.0. I'm investigating the proper fix.

@rptb1
Copy link
Member

rptb1 commented Jul 25, 2023

For the time being, we may add the -Wno-error=dangling-pointer to compiler flag, which can convert this error reporting to warning for further compiling.

Please consider that unless you are developing the MPS we recommend you do not use configure+make. See Compiling the MPS for your project.

If this is hard to do, we'd like to know more, so that we can provide a more convenient integration.

rptb1 added a commit that referenced this issue Jul 25, 2023
@rptb1
Copy link
Member

rptb1 commented Sep 10, 2023

A quick reproduction using an LXD container (in my case on Ubuntu 22):

lxc launch images:ubuntu/23.10 mps-ubuntu-mantic
lxc shell mps-ubuntu-mantic
apt-get update
apt-get install -y git gcc make
sudo -u ubuntu -i
git clone --depth 1 https://github.com/Ravenbrook/mps.git
./configure && make

The version reported by GCC is "gcc (Ubuntu 13.2.0-3ubuntu1) 13.2.0".

@thejayps thejayps added the essential Will cause failure to meet customer requirements. Assign resources. label Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Problems with builds and build automation essential Will cause failure to meet customer requirements. Assign resources.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants