Bug #20601
closedConfiguration flags are not properly propagated to assembler
Added by vo.x (Vit Ondruch) 4 months ago. Updated 4 months ago.
Description
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
.
Updated by vo.x (Vit Ondruch) 4 months ago
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.
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 4 months ago
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.
Updated by vo.x (Vit Ondruch) 4 months ago
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.
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 4 months ago ยท Edited
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 thecc1
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 tocpp
preprocessor - Passing options related to the assembler via
-Wa,
(e.g.-Wa,--generate-missing-build-notes=yes
). These get passed to thegas
assembler - Passing options related to the linker via
-Wl,
(e.g.-Wl,-z,now
). These get passed to theld
linker
The GNU make conventions however are that...
- C files are turned into Object files by running
$(CC) $(CPPFLAGS) $(CFLAGS) -c $<
.-
$(CC)
is usuallygcc
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
togcc
. 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 usuallygas
or some actual assembler - NOT a compiler driver -
$ASFLAGS
therefore has to be options spelled as they would be passed togas
; 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 togcc
, they need to be spelled in the-Wl,-z,now
format as GCC expects them, not in the-z now
format thatld
expects them
- Because
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.
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 4 months ago
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.
Updated by vo.x (Vit Ondruch) 4 months ago
kjtsanaktsidis (KJ Tsanaktsidis) wrote in #note-4:
and in fact I would go a step furthre and delete
$ASFLAGS
inconfigure.ac
andMakefile.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!)
๐
Updated by Anonymous 4 months ago
- 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]