| Message ID | 20180918165542.4691-3-miguel.ojeda.sandonis@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Compiler Attributes: (naked only, for v4.19) | expand |
On Tue, Sep 18, 2018 at 06:55:42PM +0200, Miguel Ojeda wrote: > The naked attribute is supported by at least gcc >= 4.6 (for ARM, > which is the only current user), gcc >= 8 (for x86), clang >= 3.1 > and icc >= 13. See https://godbolt.org/z/350Dyc > > Therefore, move it out of compiler-gcc.h so that the definition > is shared by all compilers. > > This also fixes Clang support for ARM32 --- 815f0ddb346c > ("include/linux/compiler*.h: make compiler-*.h mutually exclusive"). So, with this applied, does clang really build an arm32 kernel successfully? No other problem at all? And this isn't really a regression, arm32 never really worked with clang yet, right? thanks, greg k-h
Hi Greg, On Tue, Sep 18, 2018 at 7:34 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Tue, Sep 18, 2018 at 06:55:42PM +0200, Miguel Ojeda wrote: >> The naked attribute is supported by at least gcc >= 4.6 (for ARM, >> which is the only current user), gcc >= 8 (for x86), clang >= 3.1 >> and icc >= 13. See https://godbolt.org/z/350Dyc >> >> Therefore, move it out of compiler-gcc.h so that the definition >> is shared by all compilers. >> >> This also fixes Clang support for ARM32 --- 815f0ddb346c >> ("include/linux/compiler*.h: make compiler-*.h mutually exclusive"). > > So, with this applied, does clang really build an arm32 kernel > successfully? No other problem at all? And this isn't really a > regression, arm32 never really worked with clang yet, right? > To recap a bit: these two patches come from the "Compiler Attributes" series which is meant as a general improvement. Since Linus/Andrew/you didn't comment on whether you wanted or not this for 4.19, we are assuming they would go in for 4.20. However, Stefan/Nick/... wanted this for 4.19 instead, they asked me to extract these patches two separately for 4.19. I let them comment further on the status of Clang on arm32. I am going to send a v5 of the entire series without these two patches, based on -rc4 (or -next, which one do you prefer? I would say these patches should be applied early in the -next branches, so that everyone is ready for the change, given it "touches" every translation unit). Cheers, Miguel
On Tue, Sep 18, 2018 at 08:56:04PM +0200, Miguel Ojeda wrote: > Hi Greg, > > On Tue, Sep 18, 2018 at 7:34 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Tue, Sep 18, 2018 at 06:55:42PM +0200, Miguel Ojeda wrote: > >> The naked attribute is supported by at least gcc >= 4.6 (for ARM, > >> which is the only current user), gcc >= 8 (for x86), clang >= 3.1 > >> and icc >= 13. See https://godbolt.org/z/350Dyc > >> > >> Therefore, move it out of compiler-gcc.h so that the definition > >> is shared by all compilers. > >> > >> This also fixes Clang support for ARM32 --- 815f0ddb346c > >> ("include/linux/compiler*.h: make compiler-*.h mutually exclusive"). > > > > So, with this applied, does clang really build an arm32 kernel > > successfully? No other problem at all? And this isn't really a > > regression, arm32 never really worked with clang yet, right? > > > > To recap a bit: these two patches come from the "Compiler Attributes" > series which is meant as a general improvement. Ok, so that's not for regressions, that's fine. > Since Linus/Andrew/you > didn't comment on whether you wanted or not this for 4.19, we are > assuming they would go in for 4.20. However, Stefan/Nick/... wanted > this for 4.19 instead, they asked me to extract these patches two > separately for 4.19. I let them comment further on the status of Clang > on arm32. If these do not fix a regression, I don't see how they would be ready for 4.19-final. > I am going to send a v5 of the entire series without these two > patches, based on -rc4 (or -next, which one do you prefer? I would say > these patches should be applied early in the -next branches, so that > everyone is ready for the change, given it "touches" every translation > unit). That's up to whomever takes these into their tree for linux-next inclusion. If you are about to break everything, then you might consider changing your patches so they do not do that :) good luck! greg k-h
On Wed, Sep 19, 2018 at 11:14 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Tue, Sep 18, 2018 at 08:56:04PM +0200, Miguel Ojeda wrote: >> Hi Greg, >> > >> Since Linus/Andrew/you >> didn't comment on whether you wanted or not this for 4.19, we are >> assuming they would go in for 4.20. However, Stefan/Nick/... wanted >> this for 4.19 instead, they asked me to extract these patches two >> separately for 4.19. I let them comment further on the status of Clang >> on arm32. > > If these do not fix a regression, I don't see how they would be ready > for 4.19-final. Ok, I will wait a bit to send v5 until this is sorted out. [CC'd Nick, Stefan, Arnd: I just noticed the Reviewed-by/... lines were not picked as CC]. > >> I am going to send a v5 of the entire series without these two >> patches, based on -rc4 (or -next, which one do you prefer? I would say >> these patches should be applied early in the -next branches, so that >> everyone is ready for the change, given it "touches" every translation >> unit). > > That's up to whomever takes these into their tree for linux-next > inclusion. If you are about to break everything, then you might > consider changing your patches so they do not do that :) > Well, the series shouldn't break anything (famous last words :), even if everyone includes those headers. So, in theory, they *could* be applied anywhere, anytime; but given they are global changes... Cheers, Miguel
Greg Kroah-Hartman wrote on Wed, Sep 19, 2018: > > > So, with this applied, does clang really build an arm32 kernel > > > successfully? No other problem at all? And this isn't really a > > > regression, arm32 never really worked with clang yet, right? > > > > To recap a bit: these two patches come from the "Compiler Attributes" > > series which is meant as a general improvement. > > Ok, so that's not for regressions, that's fine. I've not followed so closely, in particular I'm not sure if it's the only problem with arm32 right now, but that is a regression - the general serie is meant as an improvement, but these two patches fix 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive") which was taken in 4.19-rc1 (Miguel, perhaps a Fixes: tag might help remember that?) > If these do not fix a regression, I don't see how they would be ready > for 4.19-final. So the rest of the series is not a regression and isn't ready for 4.19-final, these two would be, but only got tested by the handful of people in Cc here ; so ultimately it's your call. > > I am going to send a v5 of the entire series without these two > > patches, based on -rc4 (or -next, which one do you prefer? I would say > > these patches should be applied early in the -next branches, so that > > everyone is ready for the change, given it "touches" every translation > > unit). > > That's up to whomever takes these into their tree for linux-next > inclusion. If you are about to break everything, then you might > consider changing your patches so they do not do that :) I think that was more or less his question, there is no maintainer for these files, so who should that whomever be? :) The thing is linux took this kind of patch directly last time, but ideally it really should sink in -next for a bit... (If no-one in Cc has anything included in -next I could take them in my 9p branch, but that is quite a bit out of scope from what I advertised this branch as so I think it would be confusing ; I think it might almost be best if Miguel will keep maintaining these in the future to do his own request to inclusion in -next?)
Hi Dominique, On Thu, Sep 20, 2018 at 1:05 AM, Dominique Martinet <asmadeus@codewreck.org> wrote: > Greg Kroah-Hartman wrote on Wed, Sep 19, 2018: >> > > So, with this applied, does clang really build an arm32 kernel >> > > successfully? No other problem at all? And this isn't really a >> > > regression, arm32 never really worked with clang yet, right? >> > >> > To recap a bit: these two patches come from the "Compiler Attributes" >> > series which is meant as a general improvement. >> >> Ok, so that's not for regressions, that's fine. > > I've not followed so closely, in particular I'm not sure if it's the > only problem with arm32 right now, but that is a regression - the > general serie is meant as an improvement, but these two patches fix > 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually > exclusive") which was taken in 4.19-rc1 > > (Miguel, perhaps a Fixes: tag might help remember that?) The commit is mentioned in the commit message, although not with the Fixes: syntax -- is something now automatically parsing that? I guess it is also easier for humans to parse :) > >> If these do not fix a regression, I don't see how they would be ready >> for 4.19-final. > > So the rest of the series is not a regression and isn't ready for > 4.19-final, these two would be, but only got tested by the handful of > people in Cc here ; so ultimately it's your call. > To further clarify about the series: it is probably fine for 4.19 with the latest tests/reviews we did (v4), but it is too late for such a change in the cycle (and due to this I am taking the chance to merge the nonstring series into the Compiler Attributes one). > >> > I am going to send a v5 of the entire series without these two >> > patches, based on -rc4 (or -next, which one do you prefer? I would say >> > these patches should be applied early in the -next branches, so that >> > everyone is ready for the change, given it "touches" every translation >> > unit). >> >> That's up to whomever takes these into their tree for linux-next >> inclusion. If you are about to break everything, then you might >> consider changing your patches so they do not do that :) > > I think that was more or less his question, there is no maintainer for > these files, so who should that whomever be? :) > The thing is linux took this kind of patch directly last time, but > ideally it really should sink in -next for a bit... Yeah, Linus is (was...) aware of it, so initially I thought Linus would pick this whenever he felt it was ready. > > (If no-one in Cc has anything included in -next I could take them in my > 9p branch, but that is quite a bit out of scope from what I advertised > this branch as so I think it would be confusing ; I think it might > almost be best if Miguel will keep maintaining these in the future to do > his own request to inclusion in -next?) I can ask for my auxdisplay tree (or better, a new one) to be in -next and use that (are non-kernel.org trees allowed to be in -next, by the way?). Cheers, Miguel
Miguel Ojeda wrote on Thu, Sep 20, 2018: > > I've not followed so closely, in particular I'm not sure if it's the > > only problem with arm32 right now, but that is a regression - the > > general serie is meant as an improvement, but these two patches fix > > 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually > > exclusive") which was taken in 4.19-rc1 > > > > (Miguel, perhaps a Fixes: tag might help remember that?) > > The commit is mentioned in the commit message, although not with the > Fixes: syntax -- is something now automatically parsing that? I guess > it is also easier for humans to parse :) As far as I'm aware, the backport to stable stuff parse these to know up till how far back they should backport, but it still requires explicit Cc to the stable@vger list... (not needed here as the "bad" commit never made it to stable) I'm not aware of anything else, but as you said, while I now see you naming the commit now, I managed to miss it earlier and I was somewhat following this so it's probably also easier on humans :) > > (If no-one in Cc has anything included in -next I could take them in my > > 9p branch, but that is quite a bit out of scope from what I advertised > > this branch as so I think it would be confusing ; I think it might > > almost be best if Miguel will keep maintaining these in the future to do > > his own request to inclusion in -next?) > > I can ask for my auxdisplay tree (or better, a new one) to be in -next > and use that (are non-kernel.org trees allowed to be in -next, by the > way?). I think a new one would be great, yes. (my branch is on github, so Stephen does not appear to mind non-kernel.org trees)
On 19.09.2018 16:00, Miguel Ojeda wrote: > On Wed, Sep 19, 2018 at 11:14 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> On Tue, Sep 18, 2018 at 08:56:04PM +0200, Miguel Ojeda wrote: >>> Hi Greg, >>> >> >>> Since Linus/Andrew/you >>> didn't comment on whether you wanted or not this for 4.19, we are >>> assuming they would go in for 4.20. However, Stefan/Nick/... wanted >>> this for 4.19 instead, they asked me to extract these patches two >>> separately for 4.19. I let them comment further on the status of Clang >>> on arm32. >> >> If these do not fix a regression, I don't see how they would be ready >> for 4.19-final. Clang on arm32 worked with v4.18 when using multi_v7_defconfig -CONFIG_EFI. With 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive") it broke on v4.19-rc1. IMHO this is a regression and we should consider this two patches as a fix for it. > > Ok, I will wait a bit to send v5 until this is sorted out. > > [CC'd Nick, Stefan, Arnd: I just noticed the Reviewed-by/... lines > were not picked as CC]. Oh yeah thanks, really did not notice the discussion around v2 until you CC'd me now. -- Stefan > >> >>> I am going to send a v5 of the entire series without these two >>> patches, based on -rc4 (or -next, which one do you prefer? I would say >>> these patches should be applied early in the -next branches, so that >>> everyone is ready for the change, given it "touches" every translation >>> unit). >> >> That's up to whomever takes these into their tree for linux-next >> inclusion. If you are about to break everything, then you might >> consider changing your patches so they do not do that :) >> > > Well, the series shouldn't break anything (famous last words :), even > if everyone includes those headers. So, in theory, they *could* be > applied anywhere, anytime; but given they are global changes... > > Cheers, > Miguel
On Wed, Sep 19, 2018 at 11:00:37PM -0700, Stefan Agner wrote: > On 19.09.2018 16:00, Miguel Ojeda wrote: > > On Wed, Sep 19, 2018 at 11:14 PM, Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > >> On Tue, Sep 18, 2018 at 08:56:04PM +0200, Miguel Ojeda wrote: > >>> Hi Greg, > >>> > >> > >>> Since Linus/Andrew/you > >>> didn't comment on whether you wanted or not this for 4.19, we are > >>> assuming they would go in for 4.20. However, Stefan/Nick/... wanted > >>> this for 4.19 instead, they asked me to extract these patches two > >>> separately for 4.19. I let them comment further on the status of Clang > >>> on arm32. > >> > >> If these do not fix a regression, I don't see how they would be ready > >> for 4.19-final. > > Clang on arm32 worked with v4.18 when using multi_v7_defconfig -CONFIG_EFI. That's a narrow definition :) > With 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually > exclusive") it broke on v4.19-rc1. IMHO this is a regression and we should > consider this two patches as a fix for it. Why not just revert 815f0ddb346c instead and work to get it right for 4.20-rc1? thanks, greg k-h
On Thu, Sep 20, 2018 at 01:00:41AM +0200, Miguel Ojeda wrote: > >> I am going to send a v5 of the entire series without these two > >> patches, based on -rc4 (or -next, which one do you prefer? I would say > >> these patches should be applied early in the -next branches, so that > >> everyone is ready for the change, given it "touches" every translation > >> unit). > > > > That's up to whomever takes these into their tree for linux-next > > inclusion. If you are about to break everything, then you might > > consider changing your patches so they do not do that :) > > > > Well, the series shouldn't break anything (famous last words :), even > if everyone includes those headers. So, in theory, they *could* be > applied anywhere, anytime; but given they are global changes... It doesn't matter the "order" in which global changes are added to linux-next. If you think it does, please work with the linux-next maintainer to properly place your tree in the correct "location". thanks, greg k-h
On Thu, Sep 20, 2018 at 02:10:24AM +0200, Dominique Martinet wrote: > Miguel Ojeda wrote on Thu, Sep 20, 2018: > > > I've not followed so closely, in particular I'm not sure if it's the > > > only problem with arm32 right now, but that is a regression - the > > > general serie is meant as an improvement, but these two patches fix > > > 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually > > > exclusive") which was taken in 4.19-rc1 > > > > > > (Miguel, perhaps a Fixes: tag might help remember that?) > > > > The commit is mentioned in the commit message, although not with the > > Fixes: syntax -- is something now automatically parsing that? I guess > > it is also easier for humans to parse :) > > As far as I'm aware, the backport to stable stuff parse these to know up > till how far back they should backport, but it still requires explicit > Cc to the stable@vger list... (not needed here as the "bad" commit never > made it to stable) > I'm not aware of anything else, but as you said, while I now see you > naming the commit now, I managed to miss it earlier and I was somewhat > following this so it's probably also easier on humans :) "Fixes:" is not just for stable, we use it wherever we have a patch that we know fixes a problem introduced in another patch. For this instance, I think we should just revert the offending patch, which should resolve the issue for everyone and then you can try to redo your series to get it right the next time. Sound good? > > > (If no-one in Cc has anything included in -next I could take them in my > > > 9p branch, but that is quite a bit out of scope from what I advertised > > > this branch as so I think it would be confusing ; I think it might > > > almost be best if Miguel will keep maintaining these in the future to do > > > his own request to inclusion in -next?) > > > > I can ask for my auxdisplay tree (or better, a new one) to be in -next > > and use that (are non-kernel.org trees allowed to be in -next, by the > > way?). > > I think a new one would be great, yes. > (my branch is on github, so Stephen does not appear to mind > non-kernel.org trees) Why not just route these through Andrew? He takes lots of stuff like this for this very reason. thanks, greg k-h
Greg Kroah-Hartman wrote on Thu, Sep 20, 2018: > "Fixes:" is not just for stable, we use it wherever we have a patch that > we know fixes a problem introduced in another patch. > > For this instance, I think we should just revert the offending patch, > which should resolve the issue for everyone and then you can try to redo > your series to get it right the next time. > > Sound good? Except that 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive") itself fixes cafa0010cd51 ("Raise the minimum required gcc version to 4.6"), which breaks clang altogether (as used by example by bcc for most BPF programs, that I caught before -rc1 got released so we got both in rc1) I'm not aware of anything that would break if both were to be reverted, I have no opinion on which way to go. > Why not just route these through Andrew? He takes lots of stuff like > this for this very reason. That works for me (although it might have helped if Andrew had been in Cc at any point in this discussion...), but part of the discussion was about seriously maintaining these files, and Miguel stepped up to help with that so it could make sense to have his own tree. Frankly, after this whole episode I'd find quite helpful if "compiler stuff" (or headers maintainance in general) were to grow its own mailing list and start being considered like a proper component of the kernel. It does impact quite a few people, and it's neigh-impossible to review this stuff as things are right now with a hand-picked list of CCs, no matter how large it is -- I don't mind if it goes in -next through its own branch or through Andrew, but a proper place where folks interested in these could subscribe and test/review the patches would be awesome.
On Thu, Sep 20, 2018 at 9:37 AM Dominique Martinet <asmadeus@codewreck.org> wrote: > Greg Kroah-Hartman wrote on Thu, Sep 20, 2018: > > "Fixes:" is not just for stable, we use it wherever we have a patch that > > we know fixes a problem introduced in another patch. > > > > For this instance, I think we should just revert the offending patch, > > which should resolve the issue for everyone and then you can try to redo > > your series to get it right the next time. > > > > Sound good? > > Except that 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h > mutually exclusive") itself fixes cafa0010cd51 ("Raise the minimum > required gcc version to 4.6"), which breaks clang altogether (as used by > example by bcc for most BPF programs, that I caught before -rc1 got > released so we got both in rc1) > > I'm not aware of anything that would break if both were to be reverted, > I have no opinion on which way to go. I guess reverting them makes no difference for gcc >= 4.6. For older compilers (which were declared unsupported by cafa0010cd51), you also need https://lore.kernel.org/lkml/20180814160208.4f4dd7ca142912f5894ddddd@linux-foundation.org/ Been there, done that, happy gcc-4.1.2 ;-) Gr{oetje,eeting}s, Geert
On Thu, Sep 20, 2018 at 9:22 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > For this instance, I think we should just revert the offending patch, > which should resolve the issue for everyone and then you can try to redo > your series to get it right the next time. > > Sound good? On one hand, "reverting & retrying" is a good default policy. On the other hand, we are already in -rc4 (i.e. we lose the testing done until now --- note that in this case the revert implies a global change). So whatever makes you guys feel more comfortable. Cheers, Miguel
On Thu, Sep 20, 2018 at 9:36 AM, Dominique Martinet <asmadeus@codewreck.org> wrote: > Greg Kroah-Hartman wrote on Thu, Sep 20, 2018: >> Why not just route these through Andrew? He takes lots of stuff like >> this for this very reason. > > That works for me (although it might have helped if Andrew had been in > Cc at any point in this discussion...), but part of the discussion was Cc'ing him (and Luc too) now. I have put a bigger list of Cc people for v5 (and will simply Cc everyone for all patches). > about seriously maintaining these files, and Miguel stepped up to help > with that so it could make sense to have his own tree. > > > Frankly, after this whole episode I'd find quite helpful if "compiler > stuff" (or headers maintainance in general) were to grow its own mailing > list and start being considered like a proper component of the kernel. Yeah, the compiler headers have seen quite some changes in the last ~3 years compared to the time before so it might be a good idea: https://ojeda.io/uploads/compiler-headers-file-size.png >> It does impact quite a few people, and it's neigh-impossible to review > this stuff as things are right now with a hand-picked list of CCs, no > matter how large it is -- I don't mind if it goes in -next through its > own branch or through Andrew, but a proper place where folks interested > in these could subscribe and test/review the patches would be awesome. > I guess compiler folks would like that too. Cheers, Miguel
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 25d3dd6b2702..4d36b27214fd 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -79,14 +79,6 @@ #define __noretpoline __attribute__((indirect_branch("keep"))) #endif -/* - * it doesn't make sense on ARM (currently the only user of __naked) - * to trace naked functions because then mcount is called without - * stack and frame pointer being set up and there is no chance to - * restore the lr register to the value before mcount was called. - */ -#define __naked __attribute__((naked)) notrace - #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) #define __optimize(level) __attribute__((__optimize__(level))) diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 3525c179698c..db192becfec4 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -226,6 +226,14 @@ struct ftrace_likely_data { #define notrace __attribute__((no_instrument_function)) #endif +/* + * it doesn't make sense on ARM (currently the only user of __naked) + * to trace naked functions because then mcount is called without + * stack and frame pointer being set up and there is no chance to + * restore the lr register to the value before mcount was called. + */ +#define __naked __attribute__((naked)) notrace + #define __compiler_offsetof(a, b) __builtin_offsetof(a, b) /*
The naked attribute is supported by at least gcc >= 4.6 (for ARM, which is the only current user), gcc >= 8 (for x86), clang >= 3.1 and icc >= 13. See https://godbolt.org/z/350Dyc Therefore, move it out of compiler-gcc.h so that the definition is shared by all compilers. This also fixes Clang support for ARM32 --- 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive"). Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Eli Friedman <efriedma@codeaurora.org> Cc: Christopher Li <sparse@chrisli.org> Cc: Kees Cook <keescook@chromium.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> Cc: Joe Perches <joe@perches.com> Cc: Dominique Martinet <asmadeus@codewreck.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: linux-sparse@vger.kernel.org Suggested-by: Arnd Bergmann <arnd@arndb.de> Tested-by: Stefan Agner <stefan@agner.ch> Reviewed-by: Stefan Agner <stefan@agner.ch> Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> --- include/linux/compiler-gcc.h | 8 -------- include/linux/compiler_types.h | 8 ++++++++ 2 files changed, 8 insertions(+), 8 deletions(-)