| Message ID | 20210930180531.1190642-7-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Add support for Clang CFI | expand |
On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > With CONFIG_CFI_CLANG, the compiler changes function references to point > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > --- > include/linux/ftrace.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > index 832e65f06754..67de28464aeb 100644 > --- a/include/linux/ftrace.h > +++ b/include/linux/ftrace.h > @@ -578,9 +578,10 @@ extern void ftrace_replace_code(int enable); > extern int ftrace_update_ftrace_func(ftrace_func_t func); > extern void ftrace_caller(void); > extern void ftrace_regs_caller(void); > -extern void ftrace_call(void); > -extern void ftrace_regs_call(void); > -extern void mcount_call(void); > + > +DECLARE_ASM_FUNC_SYMBOL(ftrace_call); > +DECLARE_ASM_FUNC_SYMBOL(ftrace_regs_call); > +DECLARE_ASM_FUNC_SYMBOL(mcount_call); I'm thinking DECLARE_ASM_FUNC_SYMBOL needs a better name. It's not clear from reading it why some asm symbols need the macro and others don't. I guess it means "an asm text symbol which isn't callable from C code (not including alternatives)"? DECLARE_UNCALLED_SYMBOL() maybe?
On Tue, 5 Oct 2021 20:29:45 -0700 Josh Poimboeuf <jpoimboe@redhat.com> wrote: Thanks for Cc'ing me, as I should have been Cc'd on the original patch. > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > > With CONFIG_CFI_CLANG, the compiler changes function references to point > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. "not called from C" is a bit confusing. > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > --- > > include/linux/ftrace.h | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > > index 832e65f06754..67de28464aeb 100644 > > --- a/include/linux/ftrace.h > > +++ b/include/linux/ftrace.h > > @@ -578,9 +578,10 @@ extern void ftrace_replace_code(int enable); > > extern int ftrace_update_ftrace_func(ftrace_func_t func); > > extern void ftrace_caller(void); > > extern void ftrace_regs_caller(void); > > -extern void ftrace_call(void); > > -extern void ftrace_regs_call(void); > > -extern void mcount_call(void); > > + > > +DECLARE_ASM_FUNC_SYMBOL(ftrace_call); > > +DECLARE_ASM_FUNC_SYMBOL(ftrace_regs_call); > > +DECLARE_ASM_FUNC_SYMBOL(mcount_call); > > I'm thinking DECLARE_ASM_FUNC_SYMBOL needs a better name. It's not clear > from reading it why some asm symbols need the macro and others don't. > > I guess it means "an asm text symbol which isn't callable from C code > (not including alternatives)"? > > DECLARE_UNCALLED_SYMBOL() maybe? > That's even worse ;-) Because "called" is an assembler command in x86, and it is "called" from assembly (when you look at an objdump, it is most definitely "called"). Perhaps DECLARE_ASM_INTERNAL_SYMBOL() ? Or call it "DECLARE_ASM_MCOUNT_SYMBOL()" as "mcount" is the original name of what a compiler does when passed the -pg option, and that's exactly what those functions are. -- Steve
On Wed, Oct 06, 2021 at 09:02:49AM -0400, Steven Rostedt wrote: > On Tue, 5 Oct 2021 20:29:45 -0700 > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > Thanks for Cc'ing me, as I should have been Cc'd on the original patch. > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > > > With CONFIG_CFI_CLANG, the compiler changes function references to point > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. > > "not called from C" is a bit confusing. > > > > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > --- > > > include/linux/ftrace.h | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h > > > index 832e65f06754..67de28464aeb 100644 > > > --- a/include/linux/ftrace.h > > > +++ b/include/linux/ftrace.h > > > @@ -578,9 +578,10 @@ extern void ftrace_replace_code(int enable); > > > extern int ftrace_update_ftrace_func(ftrace_func_t func); > > > extern void ftrace_caller(void); > > > extern void ftrace_regs_caller(void); > > > -extern void ftrace_call(void); > > > -extern void ftrace_regs_call(void); > > > -extern void mcount_call(void); > > > + > > > +DECLARE_ASM_FUNC_SYMBOL(ftrace_call); > > > +DECLARE_ASM_FUNC_SYMBOL(ftrace_regs_call); > > > +DECLARE_ASM_FUNC_SYMBOL(mcount_call); > > > > I'm thinking DECLARE_ASM_FUNC_SYMBOL needs a better name. It's not clear > > from reading it why some asm symbols need the macro and others don't. > > > > I guess it means "an asm text symbol which isn't callable from C code > > (not including alternatives)"? > > > > DECLARE_UNCALLED_SYMBOL() maybe? > > > > That's even worse ;-) Because "called" is an assembler command in x86, and > it is "called" from assembly (when you look at an objdump, it is most > definitely "called"). > > Perhaps DECLARE_ASM_INTERNAL_SYMBOL() ? > > Or call it "DECLARE_ASM_MCOUNT_SYMBOL()" as "mcount" is the original name > of what a compiler does when passed the -pg option, and that's exactly what > those functions are. But this macro is used in other places as well: https://lkml.kernel.org/r/20210930180531.1190642-10-samitolvanen@google.com And many of them aren't internal to a function like the above symbols, they're actual functions that are called in other ways. DECLARE_UNCALLED_FROM_C() ?
On Wed, 6 Oct 2021 06:54:17 -0700 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > But this macro is used in other places as well: > > https://lkml.kernel.org/r/20210930180531.1190642-10-samitolvanen@google.com > > And many of them aren't internal to a function like the above symbols, > they're actual functions that are called in other ways. > > DECLARE_UNCALLED_FROM_C() ? DECLARE_NOT_CALLED_FROM_C() ;-) Straight and to the point! "uncalled" is a strange word. -- Steve
On Wed, Oct 6, 2021 at 6:02 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 5 Oct 2021 20:29:45 -0700 > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > Thanks for Cc'ing me, as I should have been Cc'd on the original patch. Sorry about that. I'll make sure you're cc'd on the next version. > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > > > With CONFIG_CFI_CLANG, the compiler changes function references to point > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. > > "not called from C" is a bit confusing. Any thoughts on how to make this less confusing? Sami
On Wed, 6 Oct 2021 09:31:04 -0700 Sami Tolvanen <samitolvanen@google.com> wrote: > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. > > > > "not called from C" is a bit confusing. > > Any thoughts on how to make this less confusing? "Not called by C code, but injected by the compiler." ? -- Steve
On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 6 Oct 2021 09:31:04 -0700 > Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point > > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. > > > > > > "not called from C" is a bit confusing. > > > > Any thoughts on how to make this less confusing? > > "Not called by C code, but injected by the compiler." > > ? Sure, sounds good to me. I'll update this in v5. Sami
On Wed, Oct 06, 2021 at 10:45:41AM -0700, Sami Tolvanen wrote: > On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > On Wed, 6 Oct 2021 09:31:04 -0700 > > Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > > > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point > > > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > > > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. > > > > > > > > "not called from C" is a bit confusing. > > > > > > Any thoughts on how to make this less confusing? > > > > "Not called by C code, but injected by the compiler." > > > > ? > > Sure, sounds good to me. I'll update this in v5. "injected by the compiler" sounds even more confusing. It almost sounds like those functions are generated by GCC, which they are most definitely not.
On Wed, 6 Oct 2021 13:43:35 -0700 Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Wed, Oct 06, 2021 at 10:45:41AM -0700, Sami Tolvanen wrote: > > On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > On Wed, 6 Oct 2021 09:31:04 -0700 > > > Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > > > > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point > > > > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > > > > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. > > > > > > > > > > "not called from C" is a bit confusing. > > > > > > > > Any thoughts on how to make this less confusing? > > > > > > "Not called by C code, but injected by the compiler." > > > > > > ? > > > > Sure, sounds good to me. I'll update this in v5. > > "injected by the compiler" sounds even more confusing. It almost sounds > like those functions are generated by GCC, which they are most > definitely not. > Heh, I was thinking of the locations that are injected (mcount / fentry) as these are just replacements for them. Those injections are added by GCC. So, continuing the bikeshedding, what about "not called by C code, but are trampolines injected as calls replacing the nops at the start of functions added by the compiler." ? -- Steve
On Wed, Oct 06, 2021 at 05:10:16PM -0400, Steven Rostedt wrote: > On Wed, 6 Oct 2021 13:43:35 -0700 > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > On Wed, Oct 06, 2021 at 10:45:41AM -0700, Sami Tolvanen wrote: > > > On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > > > On Wed, 6 Oct 2021 09:31:04 -0700 > > > > Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > > > > > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point > > > > > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > > > > > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. > > > > > > > > > > > > "not called from C" is a bit confusing. > > > > > > > > > > Any thoughts on how to make this less confusing? > > > > > > > > "Not called by C code, but injected by the compiler." > > > > > > > > ? > > > > > > Sure, sounds good to me. I'll update this in v5. > > > > "injected by the compiler" sounds even more confusing. It almost sounds > > like those functions are generated by GCC, which they are most > > definitely not. > > > > Heh, I was thinking of the locations that are injected (mcount / fentry) as > these are just replacements for them. Those injections are added by GCC. > > So, continuing the bikeshedding, what about "not called by C code, but are > trampolines injected as calls replacing the nops at the start of > functions added by the compiler." ? I'm not quite sure what that means, but I'll allow it ;-)
On Wed, Oct 6, 2021 at 2:24 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Wed, Oct 06, 2021 at 05:10:16PM -0400, Steven Rostedt wrote: > > On Wed, 6 Oct 2021 13:43:35 -0700 > > Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > > On Wed, Oct 06, 2021 at 10:45:41AM -0700, Sami Tolvanen wrote: > > > > On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > > > > > > > On Wed, 6 Oct 2021 09:31:04 -0700 > > > > > Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > > > > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote: > > > > > > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point > > > > > > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call > > > > > > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. > > > > > > > > > > > > > > "not called from C" is a bit confusing. > > > > > > > > > > > > Any thoughts on how to make this less confusing? > > > > > > > > > > "Not called by C code, but injected by the compiler." > > > > > > > > > > ? > > > > > > > > Sure, sounds good to me. I'll update this in v5. > > > > > > "injected by the compiler" sounds even more confusing. It almost sounds > > > like those functions are generated by GCC, which they are most > > > definitely not. > > > > > > > Heh, I was thinking of the locations that are injected (mcount / fentry) as > > these are just replacements for them. Those injections are added by GCC. > > > > So, continuing the bikeshedding, what about "not called by C code, but are > > trampolines injected as calls replacing the nops at the start of > > functions added by the compiler." ? > > I'm not quite sure what that means, but I'll allow it ;-) Alright, I'll go with the updated version then. I'll also rename DECLARE_ASM_FUNC_SYMBOL() to DECLARE_NOT_CALLED_FROM_C() unless someone has strong objections about that. Sami
On Wed, 6 Oct 2021 16:14:17 -0700 Sami Tolvanen <samitolvanen@google.com> wrote: > > I'm not quite sure what that means, but I'll allow it ;-) > > Alright, I'll go with the updated version then. I'll also rename > DECLARE_ASM_FUNC_SYMBOL() to DECLARE_NOT_CALLED_FROM_C() unless > someone has strong objections about that. No objection from me. I like the purple color. -- Steve
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 832e65f06754..67de28464aeb 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -578,9 +578,10 @@ extern void ftrace_replace_code(int enable); extern int ftrace_update_ftrace_func(ftrace_func_t func); extern void ftrace_caller(void); extern void ftrace_regs_caller(void); -extern void ftrace_call(void); -extern void ftrace_regs_call(void); -extern void mcount_call(void); + +DECLARE_ASM_FUNC_SYMBOL(ftrace_call); +DECLARE_ASM_FUNC_SYMBOL(ftrace_regs_call); +DECLARE_ASM_FUNC_SYMBOL(mcount_call); void ftrace_modify_all_code(int command);
With CONFIG_CFI_CLANG, the compiler changes function references to point to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them. Signed-off-by: Sami Tolvanen <samitolvanen@google.com> --- include/linux/ftrace.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)