Bug #20601: Configuration flags are not properly propagated to assembler

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 3.3.2 (2024-05-30 revision e5a195edf6) [x86_64-linux]
• Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN

[vo.x (Vit Ondruch)]

Looking into #18061, one of the issues is that the .S files are not processed with the correct flags. For example to have the CET enabled, the -fcf-protection should be used to preprocess the coroutine/amd64/Context.S.

First I thought there is something wrong on Fedora side, therefore I have proposed to export the ASFLAGS 1. However, as it turns out, $(ASFLAGS) are used by GNU make default rule and passed to $(AS). And indeed, Ruby had historically override of this rule, but it does not do this anymore since:

https://github.com/ruby/ruby/commit/091422388e943de1e67ace6faac3d71ed08c14d2
https://github.com/ruby/ruby/commit/42575570a908aac979a80b89266804c4c688dd7c

As can be seen, while previously $(AS) was used to process the .s file, it was replaced by the compiler. This however means that the .S files are not preprocessed with the $(CFLAGS), which contains -fcf-protection.

[vo.x (Vit Ondruch)]

This might be the medicine:

$ git diff
diff --git a/template/Makefile.in b/template/Makefile.in
index 2d232d7925..31e5ae5edb 100644
--- a/template/Makefile.in
+++ b/template/Makefile.in
@@ -449,7 +449,7 @@ $(srcdir)/enc/jis/props.h: enc/jis/props.kwd
 
 .$(ASMEXT).$(OBJEXT):
        @$(ECHO) assembling $<
-       $(Q) $(CC) $(ASFLAGS) -DSYMBOL_PREFIX=$(SYMBOL_PREFIX) -o $@ -c $<
+       $(Q) $(CC) $(ASFLAGS) -DSYMBOL_PREFIX=$(SYMBOL_PREFIX) $(CFLAGS) $(XCFLAGS) $(CPPFLAGS) $(COUTFLAG)$@ -o $@ -c $<
 
 .c.$(ASMEXT):
        @$(ECHO) translating $<

But I am not entirely convinced, if leaving the $(ASFLAGS) around is of any use.

[kjtsanaktsidis (KJ Tsanaktsidis)]

I think we should do what we did for the pac-ret stuff, that Florian recommended, right? Run tests against CFLAGS in configure.ac, and then define special macros?

https://github.com/ruby/ruby/blob/93b19d56de64fdee790a96ddf96fcd08d889ac93/configure.ac#L1010-L1061

I'll have a bash at doing this tonight on my x86_64 machine.

[vo.x (Vit Ondruch)]

Is something like that really needed? From my POV, the issue is that originally, there were .s files processed by Assembler, therefore the $(ASFLAGS) were needed. But now we have .S files pre-processed by compiler:

https://github.com/ruby/ruby/commit/e64f71f812324d098bed12ed68c2bc1d6e780c90
https://github.com/ruby/ruby/commit/42575570a908aac979a80b89266804c4c688dd7c

and therefore the same (or possibly extended) set of flags as for other .c files should be used. My understanding is that we should rather drop the $(ASFLAGS) usage.

[kjtsanaktsidis (KJ Tsanaktsidis)]

OK, I poked at this a bit tonight. I have quite a few thoughts but they're kind of disorganised.

The short version of this is: I think you're right; we should pass the full set of $CFLAGS $CPPFLAGS when calling $(CC) on a .S file, and in fact I would go a step furthre and delete $ASFLAGS in configure.ac and Makefile.in entirely. It's late now but I'll try and put up a PR for this tomorrow or Friday.

The long verison...

---

Detecting control flow protection with configure tests

So I started on a PR to deal with -fcf-protection in intel context.S in the same way we deal with -mbranch-protection in arm context.S: https://github.com/ruby/ruby/compare/master...KJTsanaktsidis:ruby:ktsanaktsidis/fcf_prot.

This works, but it doesn't generate the build notes in the object file to declare that it has control flow protection. I was about to go and do that (in the same way we do it for ARM here https://github.com/ruby/ruby/blob/9aa62bda46bf7f9de91a95c31fa09dafd23def37/coroutine/arm64/Context.S#L109) but then I realised that the <cet.h> header being included already does this! It has something like this in it on my system

#ifdef __CET__
 .pushsection ".note.gnu.property", "a"
 // Lots of stuff ...
#endif

So including this header automatically emits the build notes, but only if the CET macro is defined, which will only be if gcc context.S is invoked as gcc -fcf-protection=full context.S instead. We could replicate the property stuff like we do for pac-ret, but it seems pretty silly to copy stuff out of the system headers like this.

If we do what you propose and invoke gcc as an assembler with $CFLAGS, this whole thing sorts itself out (and we can drastically simplify how we handle arm64 pac-ret too!)

---

Rant about GNU Make conventions

The conventions here are all kind of messed up. GCC (and compatible compiler drivers) support...

• Passing options related to C compilation directly to the compiler (e.g. -fno-omit-frame-pointer). These get passed to the cc1 compiler process (I think)
• Passing options related to the preprocesser via -Wp, (e.g. you can spell -DFOO=1 as -Wp,-DFOO=1 if you wanted to). These get passed to cpp preprocessor
• Passing options related to the assembler via -Wa, (e.g. -Wa,--generate-missing-build-notes=yes). These get passed to the gas assembler
• Passing options related to the linker via -Wl, (e.g. -Wl,-z,now). These get passed to the ld linker

The GNU make conventions however are that...

• C files are turned into Object files by running $(CC) $(CPPFLAGS) $(CFLAGS) -c $<.
  • $(CC) is usually gcc or some compatible compiler driver.
  • $CFLAGS obviously should contain the C-compiler related flags.
  • $CPPFLAGS should contain flags related to the preprocessor. They could be spelled with the -Wp, form, but the common preprocessor flags (-I, -U, -D, etc) are not required to be and usually aren't.
  • Internally, there is both a compilation step (C to assembly) and an assembling step (assembly to object), even though I guess gcc doesn't actually generate a textual form of the assembly unless you asked for it. You might want to provide some options to this inline assembler; you can achieve this by passing e.g. -Wa,--defsym,foo=bar to gcc. However, according to the make conventions, you'd put that in $CFLAGS (since that's what gets passed to gcc).
• Assembler files are turned into Object by running $(AS) $(ASFLAGS) $<.
  • $(AS) is usually gas or some actual assembler - NOT a compiler driver
  • $ASFLAGS therefore has to be options spelled as they would be passed to gas; e.g. --defsym foo=bar instead of -Wa,--defsym,foo=bar
• Object files are turned into an executable by running $(CC) $(LDFLAGS) $^
  • Because $LDFLAGS are passed to gcc, they need to be spelled in the -Wl,-z,now format as GCC expects them, not in the -z now format that ld expects them

I guess this is all really kind of inconsistent:

• $CFLAGS are passed to gcc (makes sense)
• $ASFLAGS as passed to gas (makes sense)
• $LDFLAGS are passed to gcc, not ld (???)
• $CPPFLAGS are passed to gcc, not cpp (???)

But what you're saying is that Ruby does not assemble with $(AS) $(ASFLAGS), but rather by calling $(CC) $(ASFLAGS). That would require that $ASFLAGS be spelt as -Wa,..., not as bare options like gas (and a "vanilla" GNU make setup) would expect! I think this is one of the reasons we should delete ASFLAGS - they're not passed to the assembler in the way that they "conventionally" should be, but rather more in a manner analagous to how LDFLAGS is handled.

[kjtsanaktsidis (KJ Tsanaktsidis)]

Just one more thing: I was worried for a moment that trying to assemble with $CFLAGS would cause problems if you had some C-language specific things in cflags, but actually it seems to work fine - even if you have something like -std=gnu11 or some such.

[vo.x (Vit Ondruch)]

kjtsanaktsidis (KJ Tsanaktsidis) wrote in #note-4:

and in fact I would go a step furthre and delete $ASFLAGS in configure.ac and Makefile.in entirely. It's late now but I'll try and put up a PR for this tomorrow or Friday.

I totally support removal of $ASFLAGS. I mulled over the idea, but I was not sure. But your analysis makes sense and provides the justification.

but then I realised that the <cet.h> header being included already does this!

Correct. That is also my understanding.

If we do what you propose and invoke gcc as an assembler with $CFLAGS, this whole thing sorts itself out (and we can drastically simplify how we handle arm64 pac-ret too!)

😍

• Status changed from Open to Closed

Applied in changeset git|b18701a7ae0a71c339906ef0db4910fb43645b45.

---

Remove $(ASFLAGS) from build system and assemble with $(CFLAGS) instead

We already assemble our assembly files using the $(CC) compiler driver,
rather than the actual $(AS) assembler. This means that

• The C preprocessor gets run on the assembly file
• It's valid to pass gcc-style flags to it, like e.g.
  -mbranch-protection or -fcf-protection
• If you do so, the relevant preprocessor macros like CET get set
• If you really wanted to pass assembler flags, you would need to do
  that using -Wa,... anyway

So I think it makes sense to pass "$(XCFLAGS) $(CFLAGS) $(CPPFLAGS)" to
gcc/clang/etc when assembling, rather than passing $(ASFLAGS) (since
the flags are not actually passed to as, but cc!).

The side effect of this is that if there are mitigation flags like
-fcf-protection in $CFLAGS, then the relevant macros like CET will
be defined when assembling the files.

[Bug #20601]