| Message ID | 20170217104757.28588-1-jslaby@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/02/17 11:47, Jiri Slaby wrote: > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END, > and other macros across x86. When we have all this sorted out, this will > help to inject DWARF unwinding info by objtool later. > > So, let us use the macros this way: > * ENTRY -- start of a global function > * ENDPROC -- end of a local/global function > * GLOBAL -- start of a globally visible data symbol > * END -- end of local/global data symbol > > The goal is forcing ENTRY to emit .cfi_startproc and ENDPROC to emit > .cfi_endproc. > > This particular patch makes proper use of GLOBAL on data and ENTRY on a > function which was not the case on 4 locations. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: <x86@kernel.org> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: <xen-devel@lists.xenproject.org> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Len Brown <len.brown@intel.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: <linux-pm@vger.kernel.org> > --- > arch/x86/kernel/acpi/wakeup_64.S | 14 +++++++------- > arch/x86/kernel/head_64.S | 2 +- > arch/x86/kernel/mcount_64.S | 2 +- > arch/x86/xen/xen-head.S | 2 +- > 4 files changed, 10 insertions(+), 10 deletions(-) Xen parts: Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
* Jiri Slaby <jslaby@suse.cz> wrote: > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END, > and other macros across x86. When we have all this sorted out, this will > help to inject DWARF unwinding info by objtool later. > > So, let us use the macros this way: > * ENTRY -- start of a global function > * ENDPROC -- end of a local/global function > * GLOBAL -- start of a globally visible data symbol > * END -- end of local/global data symbol So how about using macro names that actually show the purpose, instead of importing all the crappy, historic, essentially randomly chosen debug symbol macro names from the binutils and older kernels? Something sane, like: SYM__FUNCTION_START SYM__FUNCTION_END SYM__DATA_START SYM__DATA_END ... and extend that macro namespace with any other variants we might need. We can still keep the old macro names (for a short while) to ease the transition, but for heaven's sake, if we do "cleanups" before complicating the code let's make sure the result is actually readable! Agreed? Thanks, Ingo
On 03/01/2017, 10:38 AM, Ingo Molnar wrote:
> Agreed?
Sure. I wanted to keep it minimal to see if you agree with this
direction at all. No problem to be more intrusive :).
thanks,
On Wed, 1 Mar 2017, Ingo Molnar wrote: > > * Jiri Slaby <jslaby@suse.cz> wrote: > > > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END, > > and other macros across x86. When we have all this sorted out, this will > > help to inject DWARF unwinding info by objtool later. > > > > So, let us use the macros this way: > > * ENTRY -- start of a global function > > * ENDPROC -- end of a local/global function > > * GLOBAL -- start of a globally visible data symbol > > * END -- end of local/global data symbol > > So how about using macro names that actually show the purpose, instead of > importing all the crappy, historic, essentially randomly chosen debug symbol macro > names from the binutils and older kernels? > > Something sane, like: > > SYM__FUNCTION_START Sane would be: SYM_FUNCTION_START The double underscore is just not giving any value. Thanks, tglx
* Thomas Gleixner <tglx@linutronix.de> wrote: > On Wed, 1 Mar 2017, Ingo Molnar wrote: > > > > * Jiri Slaby <jslaby@suse.cz> wrote: > > > > > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END, > > > and other macros across x86. When we have all this sorted out, this will > > > help to inject DWARF unwinding info by objtool later. > > > > > > So, let us use the macros this way: > > > * ENTRY -- start of a global function > > > * ENDPROC -- end of a local/global function > > > * GLOBAL -- start of a globally visible data symbol > > > * END -- end of local/global data symbol > > > > So how about using macro names that actually show the purpose, instead of > > importing all the crappy, historic, essentially randomly chosen debug symbol macro > > names from the binutils and older kernels? > > > > Something sane, like: > > > > SYM__FUNCTION_START > > Sane would be: > > SYM_FUNCTION_START > > The double underscore is just not giving any value. So the double underscore (at least in my view) has two advantages: 1) it helps separate the prefix from the postfix. I.e. it's a 'symbols' namespace, and a 'function start', not the 'start' of a 'symbol function'. 2) It also helps easy greppability. Try this in latest -tip: git grep e820__ To see all the E820 API calls - with no false positives! 'git grep e820_' on the other hand is a lot less reliable... But no strong feelings either way, I just try to sneak in these small namespace structure tricks when nobody's looking! ;-) Thanks, Ingo
On 03/01/2017, 11:27 AM, Ingo Molnar wrote: > But no strong feelings either way, I just try to sneak in these small namespace > structure tricks when nobody's looking! ;-) So I assume to introduce two underscores. thanks,
On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote: > >* Thomas Gleixner <tglx@linutronix.de> wrote: > >> On Wed, 1 Mar 2017, Ingo Molnar wrote: >> > >> > * Jiri Slaby <jslaby@suse.cz> wrote: >> > >> > > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, >END, >> > > and other macros across x86. When we have all this sorted out, >this will >> > > help to inject DWARF unwinding info by objtool later. >> > > >> > > So, let us use the macros this way: >> > > * ENTRY -- start of a global function >> > > * ENDPROC -- end of a local/global function >> > > * GLOBAL -- start of a globally visible data symbol >> > > * END -- end of local/global data symbol >> > >> > So how about using macro names that actually show the purpose, >instead of >> > importing all the crappy, historic, essentially randomly chosen >debug symbol macro >> > names from the binutils and older kernels? >> > >> > Something sane, like: >> > >> > SYM__FUNCTION_START >> >> Sane would be: >> >> SYM_FUNCTION_START >> >> The double underscore is just not giving any value. > >So the double underscore (at least in my view) has two advantages: > >1) it helps separate the prefix from the postfix. > >I.e. it's a 'symbols' namespace, and a 'function start', not the >'start' of a >'symbol function'. > >2) It also helps easy greppability. > >Try this in latest -tip: > > git grep e820__ > >To see all the E820 API calls - with no false positives! > >'git grep e820_' on the other hand is a lot less reliable... > >But no strong feelings either way, I just try to sneak in these small >namespace >structure tricks when nobody's looking! ;-) > >Thanks, > > Ingo This seems needlessly verbose to me and clutters the code. How about: PROC..ENDPROC, LOCALPROC..ENDPROC and DATA..ENDDATA. Clear, unambiguous and balanced.
On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote: > >* Thomas Gleixner <tglx@linutronix.de> wrote: > >> On Wed, 1 Mar 2017, Ingo Molnar wrote: >> > >> > * Jiri Slaby <jslaby@suse.cz> wrote: >> > >> > > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, >END, >> > > and other macros across x86. When we have all this sorted out, >this will >> > > help to inject DWARF unwinding info by objtool later. >> > > >> > > So, let us use the macros this way: >> > > * ENTRY -- start of a global function >> > > * ENDPROC -- end of a local/global function >> > > * GLOBAL -- start of a globally visible data symbol >> > > * END -- end of local/global data symbol >> > >> > So how about using macro names that actually show the purpose, >instead of >> > importing all the crappy, historic, essentially randomly chosen >debug symbol macro >> > names from the binutils and older kernels? >> > >> > Something sane, like: >> > >> > SYM__FUNCTION_START >> >> Sane would be: >> >> SYM_FUNCTION_START >> >> The double underscore is just not giving any value. > >So the double underscore (at least in my view) has two advantages: > >1) it helps separate the prefix from the postfix. > >I.e. it's a 'symbols' namespace, and a 'function start', not the >'start' of a >'symbol function'. > >2) It also helps easy greppability. > >Try this in latest -tip: > > git grep e820__ > >To see all the E820 API calls - with no false positives! > >'git grep e820_' on the other hand is a lot less reliable... > >But no strong feelings either way, I just try to sneak in these small >namespace >structure tricks when nobody's looking! ;-) > >Thanks, > > Ingo IMO these little "namespace tricks" especially for small common macros like we are taking about here make the code very frustrating to read, and even more to write. Noone would design a programming language that way, and things like PROC are really just substitutes for proper language features (and could even be as assembly rather than cpp macros.) When I say frustrating I mean, at least for me, full-Ballmer launch-chair-at-monitor level frustrating.
On 03/03/2017, 07:20 PM, hpa@zytor.com wrote: > On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote: >> >> * Thomas Gleixner <tglx@linutronix.de> wrote: >> >>> On Wed, 1 Mar 2017, Ingo Molnar wrote: >>>> >>>> * Jiri Slaby <jslaby@suse.cz> wrote: >>>> >>>>> This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, >> END, >>>>> and other macros across x86. When we have all this sorted out, >> this will >>>>> help to inject DWARF unwinding info by objtool later. >>>>> >>>>> So, let us use the macros this way: >>>>> * ENTRY -- start of a global function >>>>> * ENDPROC -- end of a local/global function >>>>> * GLOBAL -- start of a globally visible data symbol >>>>> * END -- end of local/global data symbol >>>> >>>> So how about using macro names that actually show the purpose, >> instead of >>>> importing all the crappy, historic, essentially randomly chosen >> debug symbol macro >>>> names from the binutils and older kernels? >>>> >>>> Something sane, like: >>>> >>>> SYM__FUNCTION_START >>> >>> Sane would be: >>> >>> SYM_FUNCTION_START >>> >>> The double underscore is just not giving any value. >> >> So the double underscore (at least in my view) has two advantages: >> >> 1) it helps separate the prefix from the postfix. >> >> I.e. it's a 'symbols' namespace, and a 'function start', not the >> 'start' of a >> 'symbol function'. >> >> 2) It also helps easy greppability. >> >> Try this in latest -tip: >> >> git grep e820__ >> >> To see all the E820 API calls - with no false positives! >> >> 'git grep e820_' on the other hand is a lot less reliable... >> >> But no strong feelings either way, I just try to sneak in these small >> namespace >> structure tricks when nobody's looking! ;-) >> >> Thanks, >> >> Ingo > > This seems needlessly verbose to me and clutters the code. > > How about: > > PROC..ENDPROC, LOCALPROC..ENDPROC and DATA..ENDDATA. Clear, unambiguous and balanced. I tried this, but: arch/x86/kernel/relocate_kernel_64.S:27:0: warning: "DATA" redefined #define DATA(offset) (KEXEC_CONTROL_CODE_MAX_SIZE+(offset)) I am not saying that I cannot fix it up. I just want to say, that these names might be too generic, especially "PROC" and "DATA". So should I really stick to these? thanks,
* hpa@zytor.com <hpa@zytor.com> wrote: > On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote: > > > >* Thomas Gleixner <tglx@linutronix.de> wrote: > > > >> On Wed, 1 Mar 2017, Ingo Molnar wrote: > >> > > >> > * Jiri Slaby <jslaby@suse.cz> wrote: > >> > > >> > > This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, > >END, > >> > > and other macros across x86. When we have all this sorted out, > >this will > >> > > help to inject DWARF unwinding info by objtool later. > >> > > > >> > > So, let us use the macros this way: > >> > > * ENTRY -- start of a global function > >> > > * ENDPROC -- end of a local/global function > >> > > * GLOBAL -- start of a globally visible data symbol > >> > > * END -- end of local/global data symbol > >> > > >> > So how about using macro names that actually show the purpose, > >instead of > >> > importing all the crappy, historic, essentially randomly chosen > >debug symbol macro > >> > names from the binutils and older kernels? > >> > > >> > Something sane, like: > >> > > >> > SYM__FUNCTION_START > >> > >> Sane would be: > >> > >> SYM_FUNCTION_START > >> > >> The double underscore is just not giving any value. > > > >So the double underscore (at least in my view) has two advantages: > > > >1) it helps separate the prefix from the postfix. > > > >I.e. it's a 'symbols' namespace, and a 'function start', not the > >'start' of a > >'symbol function'. > > > >2) It also helps easy greppability. > > > >Try this in latest -tip: > > > > git grep e820__ > > > >To see all the E820 API calls - with no false positives! > > > >'git grep e820_' on the other hand is a lot less reliable... > > > >But no strong feelings either way, I just try to sneak in these small > >namespace > >structure tricks when nobody's looking! ;-) > > > >Thanks, > > > > Ingo > > This seems needlessly verbose to me and clutters the code. > > How about: > > PROC..ENDPROC, LOCALPROC..ENDPROC and DATA..ENDDATA. Clear, unambiguous and balanced. I'm sorry, but that naming scheme is disgusing, it reminds me of BASIC nomenclature ... did we run out of underscores or what? Nor would clearly structured names clutter anything, this isn't going to be used deep inside nested code, it's going to be the single level syntactic term in addition to the symbol name itself: SYM__FUNCTION_START(some_kernel_asm_function) ... SYM__FUNCTION_END(some_kernel_asm_function) We could shorten it to 'FUNC' if length is really an issue: SYM__FUNC_START(some_kernel_asm_function) ... SYM__FUNC_END(some_kernel_asm_function) Also, 'PROC', presumably standing for 'procedure', is both ambiguous and a misnomer: - it's ambiguous with commonly used abbreviations of procfs and process - C functions are not actually 'procedures'. Procedures in PASCAL style languages denote functions that don't return any values. Most of the kernel asm functions actually do. I realize that even in C we sometimes talk about 'procedures' out of hysterical inertia, but if you check the C standards, most of them don't even use the term 'procedure'. 'function' on the other hand is totally unambiguous. Plus the lack of START/STOP (or BEGIN/END) makes it easy to commit the mistake of forgetting to add the end marker, and your naming scheme preserves that! So if we fix/extend these macros then _PLEASE_ we need to get this stupid, illogical, nonsensical, external tooling inherited naming craziness fixed first, not doubled down on... </rant> Thanks, Ingo
* hpa@zytor.com <hpa@zytor.com> wrote: > On March 1, 2017 2:27:54 AM PST, Ingo Molnar <mingo@kernel.org> wrote: > >> > So how about using macro names that actually show the purpose, instead of > >> > importing all the crappy, historic, essentially randomly chosen debug > >> > symbol macro names from the binutils and older kernels? > >> > > >> > Something sane, like: > >> > > >> > SYM__FUNCTION_START > >> > >> Sane would be: > >> > >> SYM_FUNCTION_START > >> > >> The double underscore is just not giving any value. > > > > So the double underscore (at least in my view) has two advantages: > > > > 1) it helps separate the prefix from the postfix. > > > > I.e. it's a 'symbols' namespace, and a 'function start', not the 'start' of a > > 'symbol function'. > > > > 2) It also helps easy greppability. > > > > Try this in latest -tip: > > > > git grep e820__ > > > > To see all the E820 API calls - with no false positives! > > > > 'git grep e820_' on the other hand is a lot less reliable... > > IMO these little "namespace tricks" especially for small common macros like we > are taking about here make the code very frustrating to read, and even more to > write. Noone would design a programming language that way, and things like PROC > are really just substitutes for proper language features (and could even be as > assembly rather than cpp macros.) This is a totally different thing from language keywords which needs to be short and concise for obvious reasons. Keywords of languages get nested and are used all the time, and everyone needs to know them and they need to stay out of the way. The symbol start/end macros we are talking about here are _MUCH_ less common, and they are only ever used in a single nesting level: SYM__FUNC_START(some_kernel_asm_function) ... SYM__FUNC_END(some_kernel_asm_function) Most kernel developers writing new assembly code rarely know these constructs by heart, they just look them up and carbon copy existing practices. And guess what, the 'looking them up' gets harder if the macro naming scheme is an idosyncratic leftover from long ago. Kernel developers _reading_ assembly code will know the exact purpose of the macros even less, especially if they are named in an ambiguous, illogical fashion. Furthermore, your suggestion of: > PROC..ENDPROC, LOCALPROC..ENDPROC and DATA..ENDDATA. Clear, unambiguous and > balanced. Are neither clear, not unambiguous nor balanced! I mean, they are the _exact_ opposite: - 'PROC' is actually ambiguous in the kernel source code context, as it clashes with common abbreviations of 'procfs' and 'process'. It's also an unnecessary abbreviation of a word ('procedure') that is not actually used a _single time_ in the C ISO/IEC 9899:TC2 standard - in all half thousand+ pages of it. (!) Why the hell does this have to be used in the kernel? - It's visually and semantically imbalanced, because some terms have an 'END' prefix, but there's no matching 'START' or 'BEGIN' prefix for their counterparts. This makes it easy to commit various symbol definition termination errors, like: PROC(some_kernel_asm_function) ... Here it's not obvious that it needs an ENDPROC. While if it's written as: SYM__FUNC_START(some_kernel_asm_function) ... ... it's pretty obvious at first sight that an '_END' is missing! - What you suggest also has senselessly missing underscores, which makes it _more_ cluttered and less clear. We clearly don't have addtowaitqueue() and removefromwaitqueue() function names in the kernel, right? Why should we have 'ENDPROC' and 'ENDDATA' macro names? - Hierarchical naming schemes generally tend to put the more generic category name first, not last. So it's: mutex_init() mutex_lock() mutex_unlock() mutex_trylock() It's _NOT_ the other way around: init_mutex() lock_mutex() unlock_mutex() trylock_mutex() The prefix naming scheme is easier to read both visually/typographically (because it aligns vertically in a natural fashion so it's easier to pattern match), and also semantically: because when reading it it's easy to skip the line once your brain reads the generic category of 'mutex'. But with 'ENDPROC' my brain both has to needlessly perform the following steps: - disambiguate the 'END' and the 'PROC' - fill in the missing underscore - and finally when arriving at the generic term 'PROC', discard it as uninteresting - Short names have good use in programming languages, because everyone who uses that language knows what they are and they become a visual substitute for the language element. But assembly macros are _NOT_ a new language in this sense, they are actually more similar to library function names: where brevity is actually counterintuitive and harmful, because they are ambiguous and pollute the generic namespace. If you look at C library API function name best practices you'll see that the best ones are all hierarchically named and categorized, with the more generic category put first, they are unambiguously balanced even if that makes the names longer, they are clear and use underscores. For all these reasons the naming scheme you suggest is one of the worst we could come up with! I mean, if I had to _intentionally_ engineer something as harmful as possible to readability and maintainability this would be pretty close to it... I'm upset, because even a single minute of reflection should have told you all this. I mean, IMHO it's not even a close argument: your suggested naming scheme is bleeding from half a dozen of mortal wounds... I can be convinced to drop the double underscores (I seem to be in the minority regard them), and I can be convinced that 'FUNC' is shorter and still easy to understand instead of 'FUNCTION', but other than that please stop the naming madness! Thanks, Ingo
diff --git a/arch/x86/kernel/acpi/wakeup_64.S b/arch/x86/kernel/acpi/wakeup_64.S index 50b8ed0317a3..bfd0ddefa5e8 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 +GLOBAL(saved_rbp) .quad 0 +GLOBAL(saved_rsi) .quad 0 +GLOBAL(saved_rdi) .quad 0 +GLOBAL(saved_rbx) .quad 0 -ENTRY(saved_rip) .quad 0 -ENTRY(saved_rsp) .quad 0 +GLOBAL(saved_rip) .quad 0 +GLOBAL(saved_rsp) .quad 0 -ENTRY(saved_magic) .quad 0 +GLOBAL(saved_magic) .quad 0 diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S index b467b14b03eb..7c14ab3a0f3b 100644 --- a/arch/x86/kernel/head_64.S +++ b/arch/x86/kernel/head_64.S @@ -489,7 +489,7 @@ early_gdt_descr: early_gdt_descr_base: .quad INIT_PER_CPU_VAR(gdt_page) -ENTRY(phys_base) +GLOBAL(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 b50b283f90e4..3e35675e201e 100644 --- a/arch/x86/kernel/mcount_64.S +++ b/arch/x86/kernel/mcount_64.S @@ -314,7 +314,7 @@ ENTRY(ftrace_graph_caller) retq END(ftrace_graph_caller) -GLOBAL(return_to_handler) +ENTRY(return_to_handler) subq $24, %rsp /* Save the return values */ diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S index 37794e42b67d..39ea5484763a 100644 --- a/arch/x86/xen/xen-head.S +++ b/arch/x86/xen/xen-head.S @@ -37,7 +37,7 @@ ENTRY(startup_xen) .pushsection .text .balign PAGE_SIZE -ENTRY(hypercall_page) +GLOBAL(hypercall_page) .skip PAGE_SIZE #define HYPERCALL(n) \
This is a start of series to unify use of ENTRY, ENDPROC, GLOBAL, END, and other macros across x86. When we have all this sorted out, this will help to inject DWARF unwinding info by objtool later. So, let us use the macros this way: * ENTRY -- start of a global function * ENDPROC -- end of a local/global function * GLOBAL -- start of a globally visible data symbol * END -- end of local/global data symbol The goal is forcing ENTRY to emit .cfi_startproc and ENDPROC to emit .cfi_endproc. This particular patch makes proper use of GLOBAL on data and ENTRY on a function which was not the case on 4 locations. Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: <x86@kernel.org> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Juergen Gross <jgross@suse.com> Cc: <xen-devel@lists.xenproject.org> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Len Brown <len.brown@intel.com> Cc: Pavel Machek <pavel@ucw.cz> Cc: <linux-pm@vger.kernel.org> --- arch/x86/kernel/acpi/wakeup_64.S | 14 +++++++------- arch/x86/kernel/head_64.S | 2 +- arch/x86/kernel/mcount_64.S | 2 +- arch/x86/xen/xen-head.S | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-)