Building upstream GCC with a really new GCC

[jbglaw]

Hi!

Using a really up-to-date compiler (g++ (basepoints/gcc-14-9118-g3232ebd91ed, built at 1708583405) 14.0.1 20240221 (experimental)) to build GCC including the Rest backend, we get a new warning (cf. http://toolchain.lug-owl.de/laminar/jobs/gcc-x86_64-linux/49):

../gcc/configure '--with-pkgversion=basepoints/gcc-14-9118-g3232ebd91ed, built at 1708915534' --prefix=/tmp/gcc-x86_64-linux --enable-werror-always --enable-languages=all --disable-gcov --disable-shared --disable-threads --target=x86_64-linux --without-headers --disable-multilib

make V=1 all-gcc

[...]
[all 2024-02-26 06:28:48] /var/lib/laminar/run/gcc-x86_64-linux/49/local-toolchain-install/bin/g++ -std=c++11  -fno-PIE -c  -DIN_GCC_FRONTEND -g -O2   -DIN_GCC    -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Wconditionally-supported -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Werror -Wno-unused-parameter -fno-common  -DHAVE_CONFIG_H -fno-PIE -I. -Irust -I../../gcc/gcc -I../../gcc/gcc/rust -I../../gcc/gcc/../include  -I../../gcc/gcc/../libcpp/include -I../../gcc/gcc/../libcody  -I../../gcc/gcc/../libdecnumber -I../../gcc/gcc/../libdecnumber/bid -I../libdecnumber -I../../gcc/gcc/../libbacktrace   -o rust/rust-late-name-resolver-2.0.o -MT rust/rust-late-name-resolver-2.0.o -MMD -MP -MF rust/.deps/rust-late-name-resolver-2.0.TPo -g -O2   -I ../../gcc/gcc/rust -I ../../gcc/gcc/rust/lex -I ../../gcc/gcc/rust/parse -I ../../gcc/gcc/rust/ast -I ../../gcc/gcc/rust/analysis -I ../../gcc/gcc/rust/backend -I ../../gcc/gcc/rust/expand -I ../../gcc/gcc/rust/hir/tree -I ../../gcc/gcc/rust/hir -I ../../gcc/gcc/rust/resolve -I ../../gcc/gcc/rust/util -I ../../gcc/gcc/rust/typecheck -I ../../gcc/gcc/rust/checks/lints -I ../../gcc/gcc/rust/checks/errors -I ../../gcc/gcc/rust/checks/errors/privacy -I ../../gcc/gcc/rust/checks/errors/borrowck -I ../../gcc/gcc/rust/util -I ../../gcc/gcc/rust/metadata -I ../../gcc/gcc/../libgrust ../../gcc/gcc/rust/resolve/rust-late-name-resolver-2.0.cc
[all 2024-02-26 06:29:11] ../../gcc/gcc/rust/resolve/rust-late-name-resolver-2.0.cc: In member function 'virtual void Rust::Resolver2_0::Late::visit(Rust::AST::IdentifierExpr&)':
[all 2024-02-26 06:29:11] ../../gcc/gcc/rust/resolve/rust-late-name-resolver-2.0.cc:135:17: error: '*(unsigned int*)((char*)&resolved + offsetof(tl::optional<unsigned int>,tl::optional<unsigned int>::<unnamed>.tl::detail::optional_move_assign_base<unsigned int, true>::<unnamed>.tl::detail::optional_copy_assign_base<unsigned int, true>::<unnamed>.tl::detail::optional_move_base<unsigned int, true>::<unnamed>.tl::detail::optional_copy_base<unsigned int, true>::<unnamed>.tl::detail::optional_operations_base<unsigned int>::<unnamed>.tl::detail::optional_storage_base<unsigned int, true>::<unnamed>))' may be used uninitialized [-Werror=maybe-uninitialized]
[all 2024-02-26 06:29:11]   135 |   ctx.map_usage (expr.get_node_id (), *resolved);
[all 2024-02-26 06:29:11]       |   ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[all 2024-02-26 06:29:11] ../../gcc/gcc/rust/resolve/rust-late-name-resolver-2.0.cc:125:24: note: '*(unsigned int*)((char*)&resolved + offsetof(tl::optional<unsigned int>,tl::optional<unsigned int>::<unnamed>.tl::detail::optional_move_assign_base<unsigned int, true>::<unnamed>.tl::detail::optional_copy_assign_base<unsigned int, true>::<unnamed>.tl::detail::optional_move_base<unsigned int, true>::<unnamed>.tl::detail::optional_copy_base<unsigned int, true>::<unnamed>.tl::detail::optional_operations_base<unsigned int>::<unnamed>.tl::detail::optional_storage_base<unsigned int, true>::<unnamed>))' was declared here
[all 2024-02-26 06:29:11]   125 |   tl::optional<NodeId> resolved = tl::nullopt;
[all 2024-02-26 06:29:11]       |                        ^~~~~~~~
[all 2024-02-26 06:29:14] cc1plus: all warnings being treated as errors
[all 2024-02-26 06:29:14] make[1]: *** [../../gcc/gcc/rust/Make-lang.in:441: rust/rust-late-name-resolver-2.0.o] Error 1
[all 2024-02-26 06:29:14] rm m2/gm2-compiler-boot/P2Build.mod m2/gm2-compiler-boot/P0SyntaxCheck.mod m2/gm2-compiler-boot/PCBuild.mod m2/gm2-compiler-boot/PHBuild.mod m2/gm2-compiler-boot/P1Build.mod m2/gm2-compiler-boot/P3Build.mod
[all 2024-02-26 06:29:14] make[1]: Leaving directory '/var/lib/laminar/run/gcc-x86_64-linux/49/toolchain-build/gcc'
[all 2024-02-26 06:29:15] make: *** [Makefile:5038: all-gcc] Error 2

Of course the error is provoked with --enable-werror-always, but upstream commit g:0f0ec052b4ad1fb7250a5ad1ec00d276fdc29a09 seems to suggest that fixing this warning is still a TODO.

[CohenArthur]

thanks :) marking this as a good first PR because it's a really silly mistake on my part. the fix is quite easy I think.

this is the offending code:

  tl::optional<NodeId> resolved = tl::nullopt;
  auto label = ctx.labels.get (expr.get_ident ());
  auto value = ctx.values.get (expr.get_ident ());

  if (label)
    resolved = label;
  else if (value)
    resolved = value;
  // TODO: else emit error?

  ctx.map_usage (expr.get_node_id (), *resolved);

which is indeed problematic, because if neither label nor value are valid resolution IDs, then we are accessing a nullopt's value. it's not dangerous or anything, an assertion will pop, but it will indeed be uninitialized I believe.

the correct fix would be to only call map_usage if resolved is a valid value - otherwise, emit an error or at least crash. which hopefully should get rid of the warning

[badumbatish]

thanks :) marking this as a good first PR because it's a really silly mistake on my part. the fix is quite easy I think.

this is the offending code:

  tl::optional<NodeId> resolved = tl::nullopt;
  auto label = ctx.labels.get (expr.get_ident ());
  auto value = ctx.values.get (expr.get_ident ());

  if (label)
    resolved = label;
  else if (value)
    resolved = value;
  // TODO: else emit error?

  ctx.map_usage (expr.get_node_id (), *resolved);

which is indeed problematic, because if neither label nor value are valid resolution IDs, then we are accessing a nullopt's value. it's not dangerous or anything, an assertion will pop, but it will indeed be uninitialized I believe.

the correct fix would be to only call map_usage if resolved is a valid value - otherwise, emit an error or at least crash. which hopefully should get rid of the warning

with which function should we look into to emit an error for cause a crash. Can you also point me to the code piece you've shown? Thank you!

[CohenArthur]

thanks :) marking this as a good first PR because it's a really silly mistake on my part. the fix is quite easy I think.
this is the offending code:

  tl::optional<NodeId> resolved = tl::nullopt;
  auto label = ctx.labels.get (expr.get_ident ());
  auto value = ctx.values.get (expr.get_ident ());

  if (label)
    resolved = label;
  else if (value)
    resolved = value;
  // TODO: else emit error?

  ctx.map_usage (expr.get_node_id (), *resolved);

which is indeed problematic, because if neither label nor value are valid resolution IDs, then we are accessing a nullopt's value. it's not dangerous or anything, an assertion will pop, but it will indeed be uninitialized I believe.
the correct fix would be to only call map_usage if resolved is a valid value - otherwise, emit an error or at least crash. which hopefully should get rid of the warning

with which function should we look into to emit an error for cause a crash. Can you also point me to the code piece you've shown? Thank you!

this particular piece of code is in gcc/rust/resolve/rust-late-name-resolver-2.0.cc around line 125, in the function void Late::visit (AST::IdentifierExpr &expr)

for emitting an error, you can look at using rust_error_at - there are a lot of examples in the rest of the codebase.

the error emission should look something like this:

rust_error_at(expr.get_locus (), "could not resolve identifier expression: %qs", expr.get_ident ().as_string ().c_str ());

[CohenArthur]

should I assign you the issue @badumbatish? I think it will require building g++ from the commit @jbglaw mentioned and using this to build gccrs in order to ensure it's resolved

[jbglaw]

Just needs to be a "fairly recent" GCC, from the past few days. I'd also offer to test patches by running them through my CI pipeline with --enable-werror-always.

[badumbatish]

should I assign you the issue @badumbatish? I think it will require building g++ from the commit @jbglaw mentioned and using this to build gccrs in order to ensure it's resolved

oops i didn't see this. Yes i'll try on this issue

[badumbatish]

thanks :) marking this as a good first PR because it's a really silly mistake on my part. the fix is quite easy I think.
this is the offending code:

  tl::optional<NodeId> resolved = tl::nullopt;
  auto label = ctx.labels.get (expr.get_ident ());
  auto value = ctx.values.get (expr.get_ident ());

  if (label)
    resolved = label;
  else if (value)
    resolved = value;
  // TODO: else emit error?

  ctx.map_usage (expr.get_node_id (), *resolved);

which is indeed problematic, because if neither label nor value are valid resolution IDs, then we are accessing a nullopt's value. it's not dangerous or anything, an assertion will pop, but it will indeed be uninitialized I believe.
the correct fix would be to only call map_usage if resolved is a valid value - otherwise, emit an error or at least crash. which hopefully should get rid of the warning

with which function should we look into to emit an error for cause a crash. Can you also point me to the code piece you've shown? Thank you!

this particular piece of code is in gcc/rust/resolve/rust-late-name-resolver-2.0.cc around line 125, in the function void Late::visit (AST::IdentifierExpr &expr)

for emitting an error, you can look at using rust_error_at - there are a lot of examples in the rest of the codebase.

the error emission should look something like this:

rust_error_at(expr.get_locus (), "could not resolve identifier expression: %qs", expr.get_ident ().as_string ().c_str ());

I just realized the make error output shows the file crash location ....

[CohenArthur]

Closed in #2895