Skip to content

openssf-compiler-options: redesign gcc integration - #27815

Merged
smoser merged 3 commits into
wolfi-dev:mainfrom
xnox:wrap-gcc
Sep 12, 2024
Merged

smoser merged 3 commits into
wolfi-dev:mainfrom
xnox:wrap-gcc

Conversation

@xnox

@xnox xnox commented Sep 6, 2024 •

Copy link
Copy Markdown
Member

When no specs are specified gcc self-creates "built-in" default specs in memory.

If /usr/lib/gcc/arch/version/specs exists, it loads that, instead of generating "built-in" default specs. And cruitially does not generate "built-in" default specs at all.

Specs passed in via "-specs=" command are looked up in the same paths as "specs", however gcc still generates "built-in" default specs.

Also /usr/lib/gcc/arch/version/specs file is not allowed to use include commands, and is thus must be static / fully self-contained. The general recomendation is to start with gcc -dumpspecs tweak that, and keep that updated. But that is very error prone, as gcc default specs have a lot of boilerplate that is compiler configure flags dependant.

There is no filepath to drop in specs that are loaded the same way as -specs=<file> are loaded (after default "built-in" ones are generated, with include commands allowed).

The scary part is that incompete specs still cause compiler to sort of work, but do result in 'gcc_s' not being linked in, hence the many build-failures from the mass rebuilds with openssf-compiler-options enabled. And that many software manages to build fine, especially those with resiliant built systems that emitted fully qualified linker commands to gcc. Lack of 'gcc_s' in the default link commands resulted in cryptic build failures such as "unable to add library, undefined symbol _Unwind_Rewind, DSO libgcc_s missing from command line" and the like.

None of this documented in gcc documentation for Spec Files, and is only obvious from reading gcc source code. See this line that loads specs, without built-in defaults and prevents using "include" commands with the second argument set to true.

This is a very unfortunate set of circumstances. Patching compiler specs loading is trivial in above linked code, but acceptance by upstream is unknown at this point. I will attempt a submission and will link it here at a later date. Thus resort to shell wrapper in a higher priority path directory for now.

This should fix many build failures observed when trying to mass enable openssf-compiler-options. One example is that cmake failed to even configure its trivial bootstrap-cmake without this fix. Also trivial package test added that shows problem with the current build, which is resolved with wrapper solution.

@xnox
xnox force-pushed the wrap-gcc branch 2 times, most recently from fe4bcef to fb44fac Compare September 6, 2024 19:13

@smoser smoser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I right that you're relying on /usr/local/bin being in front of /usr/bin in the PATH for this to work?

That will be a confusing thing to debug at some point.

Comment thread openssf-compiler-options.yaml Resolved
@xnox

xnox commented Sep 6, 2024

Copy link
Copy Markdown
Member Author

Am I right that you're relying on /usr/local/bin being in front of /usr/bin in the PATH for this to work?

That will be a confusing thing to debug at some point.

Yes.

Options to patch gcc is to add a new file api "specs.overlay" which allow includes like so:

 /* Process any built-in specs overlay, allow include_noerr */
specs_file = find_a_file (&startfile_prefixes, "specs.overlay", R_OK, true);
if (specs_file != 0 && access (specs_file, R_OK) == 0)
   read_specs (specs_file, false, false);

Just before user provided specs. Which can then contain "%include_noerr <openssf.spec>"

Or to make gcc always load built-ins, and then process "specs" with include commands allowed like so

diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index ae1d80fe00a..d6e61fb8018 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -8464,12 +8464,18 @@ driver::set_up_specs () const
 			   accel_dir_suffix, dir_separator_str, NULL);
   just_machine_suffix = concat (spec_machine, dir_separator_str, NULL);

+  /* Always init built-in specs, as all followups can then
+     append/remove/rename. This also makes the existing Spec documentation true
+     for both command-line -specs provided files, and those added to the
+     <startfiles>/specs location */
+  init_spec ();
+
   specs_file = find_a_file (&startfile_prefixes, "specs", R_OK, true);
-  /* Read the specs file unless it is a default one.  */
-  if (specs_file != 0 && strcmp (specs_file, "specs"))
-    read_specs (specs_file, true, false);
-  else
-    init_spec ();
+
+  /* Read the specs file, these days empty, thus allow #include_noerr
+     commands */
+  if (specs_file != 0)
+    read_specs (specs_file, false, false);

or to execute gcc -dumpspecs on every build, patch them, and ship them in a subpackage. But then %include_noerr will still not be allowed, which I was hoping to use later on for pkginformation notes.

smoser added a commit that referenced this pull request Sep 6, 2024
I was unable to get this to buidl without the provided patch.
Getting -Wno-error=maybe-uninitialized into the
compiler flags was not enough.

There are other examples in binaryen's git where this
pragma approach is used.

I could not get it to build with openssf-compiler-options
enabled, very close.  By adding

    vars:
      no-error-flags: -Wno-error=conversion \
        -Wno-error=float-conversion \
        -Wno-error=format-nonliteral \
        -Wno-error=maybe-uninitialized \
        -Wno-error=stringop-overflow

and then adding to the cmake/configure opts

  -DCMAKE_CXX_FLAGS="${{vars.no-error-flags}}"

we can get to an _Unwind_Resume linker error.
Presumably that will be fixed with something like
#27815

@smoser smoser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this.
Can we run some reverse dependencies to verify that we didn't break anything and that they work as expected?

When no specs are specified gcc self-creates "built-in" default specs
in memory.

If /usr/lib/gcc/arch/version/specs exists, it loads that, instead of
generating "built-in" default specs. And cruitially does not generate
"built-in" default specs at all.

Specs passed in via "-specs=<file>" command are looked up in the same
paths as "specs", however gcc still generates "built-in" default
specs.

Also /usr/lib/gcc/arch/version/specs file is not allowed to use
include commands, and is thus must be static / fully
self-contained. The general recomendation is to start with `gcc
-dumpspecs` tweak that, and keep that updated. But that is very error
prone, as gcc default specs have a lot of boilerplate that is compiler
configure flags dependant.

There is no filepath to drop in specs that are loaded the same way as
`-specs=<file>` are loaded (after default "built-in" ones are
generated, with include commands allowed).

The scary part is that incompete specs still cause compiler to sort of
work, but do result in 'gcc_s' not being linked in, hence the many
build-failures from the mass rebuilds with openssf-compiler-options
enabled. And that many software manages to build fine, especially
those with resiliant built systems that emitted fully qualified linker
commands to gcc. Lack of 'gcc_s' in the default link commands resulted
in cryptic build failures such as "unable to add library, undefined
symbol _Unwind_Rewind, DSO libgcc_s missing from command line" and the
like.

None of this documented in gcc documentation for [Spec Files], and is
only obvious from reading gcc source code. See
[this](https://github.com/gcc-mirror/gcc/blob/1a1f818c60636f219edd2cb93485aa08e1a36c88/gcc/gcc.cc#L8470)
line that loads specs, without built-in defaults and prevents using
"include" commands with the second argument set to true.

This is a very unfortunate set of circumstances. Patching compiler
specs loading is trivial in above linked code, but acceptance by
upstream is unknown at this point. I will attempt a submission and
will link it here at a later date. Thus resort to shell wrapper in a
higher priority path directory for now.

This should fix many build failures observed when trying to mass
enable openssf-compiler-options. One example is that cmake failed to
even configure its trivial bootstrap-cmake without this fix. Also
trivial package test added that shows problem with the current build,
which is resolved with wrapper solution.

[Spec Files]: https://gcc.gnu.org/onlinedocs/gcc/Spec-Files.html
@smoser
smoser merged commit 1c7c65e into wolfi-dev:main Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants