| Message ID | 20170320123222.15453-2-jslaby@suse.cz (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote: > This is a start of series to cleanup macros used for starting functions, > data, globals etc. across x86. When we have all this sorted out, this > will help to inject DWARF unwinding info by objtool later. > > The goal is forcing SYM_FUNC_START to emit .cfi_startproc and > SYM_FUNC_END to emit .cfi_endproc. Automatically at best. Do we still want to emit .cfi_startproc/endproc from the macro? From our last discussion, that seemed to be up in the air. https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble What did you think about making CFI read-only for .c object files and write-only for .S object files?
On 03/20/2017, 02:32 PM, Josh Poimboeuf wrote: > On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote: >> This is a start of series to cleanup macros used for starting functions, >> data, globals etc. across x86. When we have all this sorted out, this >> will help to inject DWARF unwinding info by objtool later. >> >> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and >> SYM_FUNC_END to emit .cfi_endproc. Automatically at best. > > Do we still want to emit .cfi_startproc/endproc from the macro? From > our last discussion, that seemed to be up in the air. > > https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble "Automatically at best" above means "completely from objtool". I am still uncertain whether it will work 100% or we would have to help by generating some pieces from the added macros. In particular, the ALIASes are evil which cause harm here: fun_alias: fun: <code> .size fun, .-fun .type fun STT_FUNC .size fun_alias, .-fun_alias .type fun_alias STT_FUNC Both cannot create (overlapping) .cfi_startproc/endproc, only the inner shall. But it seems so far, that we might be able to deal with all of that from objtool... (I have not been thinking about this particular thing deep enough yet.) Some sort of "from the last label that is marked as STT_FUNC till its .size" might work. > What did you think about making CFI read-only for .c object files and > write-only for .S object files? There are those functions like sync_core() or native_save_fl() with inline asm. And they seem to need a) read-write support, or b) manual annotation. I would like to avoid b) for sure. thanks,
On Mon, Mar 20, 2017 at 04:32:09PM +0100, Jiri Slaby wrote: > On 03/20/2017, 02:32 PM, Josh Poimboeuf wrote: > > On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote: > >> This is a start of series to cleanup macros used for starting functions, > >> data, globals etc. across x86. When we have all this sorted out, this > >> will help to inject DWARF unwinding info by objtool later. > >> > >> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and > >> SYM_FUNC_END to emit .cfi_endproc. Automatically at best. > > > > Do we still want to emit .cfi_startproc/endproc from the macro? From > > our last discussion, that seemed to be up in the air. > > > > https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble > > "Automatically at best" above means "completely from objtool". I am > still uncertain whether it will work 100% or we would have to help by > generating some pieces from the added macros. In particular, the ALIASes > are evil which cause harm here: > > fun_alias: > fun: > <code> > .size fun, .-fun > .type fun STT_FUNC > .size fun_alias, .-fun_alias > .type fun_alias STT_FUNC > > Both cannot create (overlapping) .cfi_startproc/endproc, only the inner > shall. > > But it seems so far, that we might be able to deal with all of that from > objtool... (I have not been thinking about this particular thing deep > enough yet.) Some sort of "from the last label that is marked as > STT_FUNC till its .size" might work. Ok. > > What did you think about making CFI read-only for .c object files and > > write-only for .S object files? > > There are those functions like sync_core() or native_save_fl() with > inline asm. And they seem to need a) read-write support, or b) manual > annotation. I would like to avoid b) for sure. Ah, so I guess those inline asm functions cause problems because they muck with the stack pointer with pushes and pops? I don't think manual annotation of inline asm would be so bad. IIUC, it would only mean replacing the pushes and pops with a macro which does the CFI-annotated version, like PUSH_CFI and POP_CFI. And the benefit would be that objtool doesn't have to try to rewrite a bunch of .c object files. Objtool read-write worries me because it gives more responsibility to objtool. It could be tricky to insert CFI instructions within the ones already created by gcc. Also, while unlikely, a bug in objtool could theoretically corrupt an object file and brick the kernel. Also I wonder how all those extra file writes would affect build performance. If at all possible, I would rather objtool stay out of the way of the compiler and let gcc do its job of generating CFI.
Hi! > -ENTRY(saved_rbp) .quad 0 > -ENTRY(saved_rsi) .quad 0 > -ENTRY(saved_rdi) .quad 0 > -ENTRY(saved_rbx) .quad 0 > +SYM_DATA_START(saved_rbp) .quad 0 > +SYM_DATA_START(saved_rsi) .quad 0 > +SYM_DATA_START(saved_rdi) .quad 0 > +SYM_DATA_START(saved_rbx) .quad 0 Does it make sense to call it SYM_DATA_*START* when there's no corresponding end? Plus... it looks like saved_rsi (and friends) are only used inside wakeup_64.S. Could we just delete the "ENTRY" annotations? Thanks, Pavel
* Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > -ENTRY(saved_rbp) .quad 0 > > -ENTRY(saved_rsi) .quad 0 > > -ENTRY(saved_rdi) .quad 0 > > -ENTRY(saved_rbx) .quad 0 > > +SYM_DATA_START(saved_rbp) .quad 0 > > +SYM_DATA_START(saved_rsi) .quad 0 > > +SYM_DATA_START(saved_rdi) .quad 0 > > +SYM_DATA_START(saved_rbx) .quad 0 > > Does it make sense to call it SYM_DATA_*START* when there's no > corresponding end? That looks like a bug - I think we should strive for them to always be in pairs. Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' SYM_*_START() uses? This could be done by emitting debug data into a special section and then analyzing that section for unpaired entries. The section can be discarded in the final link, it won't show up in the kernel image. We don't ever nest symbols, right? > Plus... it looks like saved_rsi (and friends) are only used inside > wakeup_64.S. Could we just delete the "ENTRY" annotations? That appears to make sense as well. Thanks, Ingo
On 03/22/2017, 08:25 AM, Ingo Molnar wrote: > > * Pavel Machek <pavel@ucw.cz> wrote: > >> Hi! >> >>> -ENTRY(saved_rbp) .quad 0 >>> -ENTRY(saved_rsi) .quad 0 >>> -ENTRY(saved_rdi) .quad 0 >>> -ENTRY(saved_rbx) .quad 0 >>> +SYM_DATA_START(saved_rbp) .quad 0 >>> +SYM_DATA_START(saved_rsi) .quad 0 >>> +SYM_DATA_START(saved_rdi) .quad 0 >>> +SYM_DATA_START(saved_rbx) .quad 0 >> >> Does it make sense to call it SYM_DATA_*START* when there's no >> corresponding end? > > That looks like a bug - I think we should strive for them to always be in pairs. > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' > SYM_*_START() uses? This could be done by emitting debug data into a special > section and then analyzing that section for unpaired entries. The section can be > discarded in the final link, it won't show up in the kernel image. It should be easier than that. No introduction of other info needed -- every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should be a bug now. > We don't ever nest symbols, right? AFAI could see so far, correct. >> Plus... it looks like saved_rsi (and friends) are only used inside >> wakeup_64.S. Could we just delete the "ENTRY" annotations? > > That appears to make sense as well. +1, will fix this. thanks,
* Jiri Slaby <jslaby@suse.cz> wrote: > On 03/22/2017, 08:25 AM, Ingo Molnar wrote: > > > > * Pavel Machek <pavel@ucw.cz> wrote: > > > >> Hi! > >> > >>> -ENTRY(saved_rbp) .quad 0 > >>> -ENTRY(saved_rsi) .quad 0 > >>> -ENTRY(saved_rdi) .quad 0 > >>> -ENTRY(saved_rbx) .quad 0 > >>> +SYM_DATA_START(saved_rbp) .quad 0 > >>> +SYM_DATA_START(saved_rsi) .quad 0 > >>> +SYM_DATA_START(saved_rdi) .quad 0 > >>> +SYM_DATA_START(saved_rbx) .quad 0 > >> > >> Does it make sense to call it SYM_DATA_*START* when there's no > >> corresponding end? > > > > That looks like a bug - I think we should strive for them to always be in pairs. > > > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' > > SYM_*_START() uses? This could be done by emitting debug data into a special > > section and then analyzing that section for unpaired entries. The section can be > > discarded in the final link, it won't show up in the kernel image. > > It should be easier than that. No introduction of other info needed -- > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should > be a bug now. I'm all for that! Can we detect double ends as well - i.e. do a build check of the full syntax of these symbol definition primitives? Thanks, Ingo
Hi, On 03/21/2017, 03:08 PM, Pavel Machek wrote: >> -ENTRY(saved_rbp) .quad 0 >> -ENTRY(saved_rsi) .quad 0 >> -ENTRY(saved_rdi) .quad 0 >> -ENTRY(saved_rbx) .quad 0 >> +SYM_DATA_START(saved_rbp) .quad 0 >> +SYM_DATA_START(saved_rsi) .quad 0 >> +SYM_DATA_START(saved_rdi) .quad 0 >> +SYM_DATA_START(saved_rbx) .quad 0 > > Does it make sense to call it SYM_DATA_*START* when there's no > corresponding end? > > Plus... it looks like saved_rsi (and friends) are only used inside > wakeup_64.S. Could we just delete the "ENTRY" annotations? So, now I have: === linkage.h === /* SYM_DATA_SIMPLE -- start+end wrapper around simple global data */ #ifndef SYM_DATA_SIMPLE #define SYM_DATA_SIMPLE(name, data) \ SYM_DATA_START(name) ASM_NL \ data ASM_NL \ SYM_DATA_END(name) #endif /* SYM_DATA_SIMPLE_LOCAL -- start+end wrapper around simple local data */ #ifndef SYM_DATA_SIMPLE_LOCAL #define SYM_DATA_SIMPLE_LOCAL(name, data) \ SYM_DATA_START_LOCAL(name) ASM_NL \ data ASM_NL \ SYM_DATA_END(name) #endif === wakeup_64.S === SYM_DATA_SIMPLE_LOCAL(saved_rbp, .quad 0) SYM_DATA_SIMPLE_LOCAL(saved_rsi, .quad 0) SYM_DATA_SIMPLE_LOCAL(saved_rdi, .quad 0) SYM_DATA_SIMPLE_LOCAL(saved_rbx, .quad 0) SYM_DATA_SIMPLE_LOCAL(saved_rip, .quad 0) SYM_DATA_SIMPLE_LOCAL(saved_rsp, .quad 0) SYM_DATA_SIMPLE_LOCAL(saved_magic, .quad 0) === original === 10: 0000000000000060 0 NOTYPE GLOBAL DEFAULT 3 saved_magic 11: 0000000000000050 0 NOTYPE GLOBAL DEFAULT 3 saved_rsp 12: 0000000000000030 0 NOTYPE GLOBAL DEFAULT 3 saved_rbx 13: 0000000000000020 0 NOTYPE GLOBAL DEFAULT 3 saved_rdi 14: 0000000000000010 0 NOTYPE GLOBAL DEFAULT 3 saved_rsi 15: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 3 saved_rbp 16: 0000000000000040 0 NOTYPE GLOBAL DEFAULT 3 saved_rip === new === 4: 0000000000000030 8 OBJECT LOCAL DEFAULT 3 saved_magic 6: 0000000000000028 8 OBJECT LOCAL DEFAULT 3 saved_rsp 7: 0000000000000018 8 OBJECT LOCAL DEFAULT 3 saved_rbx 8: 0000000000000010 8 OBJECT LOCAL DEFAULT 3 saved_rdi 9: 0000000000000008 8 OBJECT LOCAL DEFAULT 3 saved_rsi 10: 0000000000000000 8 OBJECT LOCAL DEFAULT 3 saved_rbp 11: 0000000000000020 8 OBJECT LOCAL DEFAULT 3 saved_rip === EOF === BTW, ENTRY() aligned the data to 2^4 = 16 as we can see in the original. But I see no point aligning data like this. thanks,
On Wed 2017-03-22 13:06:54, Jiri Slaby wrote: > Hi, > > On 03/21/2017, 03:08 PM, Pavel Machek wrote: > >> -ENTRY(saved_rbp) .quad 0 > >> -ENTRY(saved_rsi) .quad 0 > >> -ENTRY(saved_rdi) .quad 0 > >> -ENTRY(saved_rbx) .quad 0 > >> +SYM_DATA_START(saved_rbp) .quad 0 > >> +SYM_DATA_START(saved_rsi) .quad 0 > >> +SYM_DATA_START(saved_rdi) .quad 0 > >> +SYM_DATA_START(saved_rbx) .quad 0 > > > > Does it make sense to call it SYM_DATA_*START* when there's no > > corresponding end? > > > > Plus... it looks like saved_rsi (and friends) are only used inside > > wakeup_64.S. Could we just delete the "ENTRY" annotations? > > > So, now I have: > > === linkage.h === > > /* SYM_DATA_SIMPLE -- start+end wrapper around simple global data */ > #ifndef SYM_DATA_SIMPLE > #define SYM_DATA_SIMPLE(name, data) \ > SYM_DATA_START(name) ASM_NL \ > data ASM_NL \ > SYM_DATA_END(name) > #endif > > /* SYM_DATA_SIMPLE_LOCAL -- start+end wrapper around simple local data */ > #ifndef SYM_DATA_SIMPLE_LOCAL > #define SYM_DATA_SIMPLE_LOCAL(name, data) \ > SYM_DATA_START_LOCAL(name) ASM_NL \ > data ASM_NL \ > SYM_DATA_END(name) > #endif > > === wakeup_64.S === > > SYM_DATA_SIMPLE_LOCAL(saved_rbp, .quad 0) > SYM_DATA_SIMPLE_LOCAL(saved_rsi, .quad 0) > SYM_DATA_SIMPLE_LOCAL(saved_rdi, .quad 0) > SYM_DATA_SIMPLE_LOCAL(saved_rbx, .quad 0) > > SYM_DATA_SIMPLE_LOCAL(saved_rip, .quad 0) > SYM_DATA_SIMPLE_LOCAL(saved_rsp, .quad 0) > > SYM_DATA_SIMPLE_LOCAL(saved_magic, .quad 0) > > === original === > > 10: 0000000000000060 0 NOTYPE GLOBAL DEFAULT 3 saved_magic > 11: 0000000000000050 0 NOTYPE GLOBAL DEFAULT 3 saved_rsp > 12: 0000000000000030 0 NOTYPE GLOBAL DEFAULT 3 saved_rbx > 13: 0000000000000020 0 NOTYPE GLOBAL DEFAULT 3 saved_rdi > 14: 0000000000000010 0 NOTYPE GLOBAL DEFAULT 3 saved_rsi > 15: 0000000000000000 0 NOTYPE GLOBAL DEFAULT 3 saved_rbp > 16: 0000000000000040 0 NOTYPE GLOBAL DEFAULT 3 saved_rip > > === new === > > 4: 0000000000000030 8 OBJECT LOCAL DEFAULT 3 saved_magic > 6: 0000000000000028 8 OBJECT LOCAL DEFAULT 3 saved_rsp > 7: 0000000000000018 8 OBJECT LOCAL DEFAULT 3 saved_rbx > 8: 0000000000000010 8 OBJECT LOCAL DEFAULT 3 saved_rdi > 9: 0000000000000008 8 OBJECT LOCAL DEFAULT 3 saved_rsi > 10: 0000000000000000 8 OBJECT LOCAL DEFAULT 3 saved_rbp > 11: 0000000000000020 8 OBJECT LOCAL DEFAULT 3 saved_rip > > === EOF === > > BTW, ENTRY() aligned the data to 2^4 = 16 as we can see in the original. > But I see no point aligning data like this. Yep, that's a bug, too. I guess it hurts even more on 32-bit.... Thanks for fixing this, Pavel
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S index e1721dafbcb1..73d7ff0b125c 100644 --- a/arch/x86/entry/entry_64_compat.S +++ b/arch/x86/entry/entry_64_compat.S @@ -342,8 +342,7 @@ ENTRY(entry_INT80_compat) jmp restore_regs_and_iret END(entry_INT80_compat) - ALIGN -GLOBAL(stub32_clone) +SYM_FUNC_START(stub32_clone) /* * The 32-bit clone ABI is: clone(..., int tls_val, int *child_tidptr). * The 64-bit clone ABI is: clone(..., int *child_tidptr, int tls_val). diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S index 50b8ed0317a3..0b5a5573f57d 100644 --- a/arch/x86/kernel/acpi/wakeup_64.S +++ b/arch/x86/kernel/acpi/wakeup_64.S @@ -125,12 +125,12 @@ ENTRY(do_suspend_lowlevel) ENDPROC(do_suspend_lowlevel) .data -ENTRY(saved_rbp) .quad 0 -ENTRY(saved_rsi) .quad 0 -ENTRY(saved_rdi) .quad 0 -ENTRY(saved_rbx) .quad 0 +SYM_DATA_START(saved_rbp) .quad 0 +SYM_DATA_START(saved_rsi) .quad 0 +SYM_DATA_START(saved_rdi) .quad 0 +SYM_DATA_START(saved_rbx) .quad 0 -ENTRY(saved_rip) .quad 0 -ENTRY(saved_rsp) .quad 0 +SYM_DATA_START(saved_rip) .quad 0 +SYM_DATA_START(saved_rsp) .quad 0 -ENTRY(saved_magic) .quad 0 +SYM_DATA_START(saved_magic) .quad 0 diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S index ac9d327d2e42..e093a804f1fb 100644 --- a/arch/x86/kernel/head_64.S +++ b/arch/x86/kernel/head_64.S @@ -487,7 +487,7 @@ early_gdt_descr: early_gdt_descr_base: .quad INIT_PER_CPU_VAR(gdt_page) -ENTRY(phys_base) +SYM_DATA_START(phys_base) /* This must match the first entry in level2_kernel_pgt */ .quad 0x0000000000000000 EXPORT_SYMBOL(phys_base) diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S index 7b0d3da52fb4..2b4d7045e823 100644 --- a/arch/x86/kernel/mcount_64.S +++ b/arch/x86/kernel/mcount_64.S @@ -320,7 +320,7 @@ ENTRY(ftrace_graph_caller) retq END(ftrace_graph_caller) -GLOBAL(return_to_handler) +SYM_FUNC_START(return_to_handler) subq $24, %rsp /* Save the return values */