x86: introduce ENTRY(KPROBE)_X86 assembly helpers to catch unbalanced declaration

It's usefull to catch unbalanced, missed or mixed declarations of ENTRY and KPROBES. These macros would help a bit (at least I hope so).

For example the following code would compile without problems

        ENTRY_X86(mcount)
                retq
        END_X86(mcount)

But if you forget and mix the following form

        ENTRY_X86(mcount)
                retq
        END(mcount)

        ENTRY_X86(ftrace_caller)

The assembler will issue the following message:
Error: ENTRY_X86/KPROBE_X86 unbalanced,missed,mixed

Actually the checking is performed at every _X86 macro so maybe it's good idea to put ENTRY_KPROBE_FINAL_X86 at the end of .S file to be sure you didn't miss anything.

---
 arch/x86/include/asm/linkage.h |   60 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

Index: linux-2.6.git/arch/x86/include/asm/linkage.h
===================================================================
@@ -57,5 +57,65 @@
 #define __ALIGN_STR ".align 16,0x90"
 #endif
 
+/*
+ * to check ENTRY_X86/END_X86 and
+ * KPROBE_ENTRY_X86/KPROBE_END_X86
+ * unbalanced-missed-mixed appearance
+ */
+#define __set_entry_x86		.set ENTRY_X86_IN, 0
+#define __unset_entry_x86	.set ENTRY_X86_IN, 1
+#define __set_kprobe_x86	.set KPROBE_X86_IN, 0
+#define __unset_kprobe_x86	.set KPROBE_X86_IN, 1
+
+#define __macro_err_x86 .error "ENTRY_X86/KPROBE_X86 unbalanced,missed,mixed"
+
+#define __check_entry_x86	\
+	.ifdef ENTRY_X86_IN;	\
+	.ifeq ENTRY_X86_IN;	\
+	__macro_err_x86;	\
+	.abort;			\
+	.endif;			\
+	.endif
+
+#define __check_kprobe_x86	\
+	.ifdef KPROBE_X86_IN;	\
+	.ifeq KPROBE_X86_IN;	\
+	__x86_macro_err;	\
+	.abort;			\
+	.endif;			\
+	.endif
+
+#define __check_entry_kprobe_x86	\
+	__check_entry_x86;		\
+	__check_kprobe_x86
+
+#define ENTRY_KPROBE_FINAL_X86 __check_entry_kprobe_x86
+
+#define ENTRY_X86(name)			\
+	__check_entry_kprobe_x86;	\
+	__set_entry_x86;		\
+	.globl name;			\
+	__ALIGN;			\
+	name:
+
+#define END_X86(name)			\
+	__unset_entry_x86;		\
+	__check_entry_kprobe_x86;	\
+	.size name, .-name
+
+#define KPROBE_ENTRY_X86(name)		\
+	__check_entry_kprobe_x86;	\
+	__set_kprobe_x86;		\
+	.pushsection .kprobes.text, "ax"; \
+	.globl name;			\
+	__ALIGN;			\
+	name:
+
+#define KPROBE_END_X86(name)		\
+	__unset_kprobe_x86;		\
+	__check_entry_kprobe_x86;	\
+	.size name, .-name;		\
+	.popsection
+
 #endif /* _ASM_X86_LINKAGE_H */
 

[Cyrill Gorcunov - Sun, Nov 23, 2008 at 07:57:11PM +0300]

| It's usefull to catch unbalanced, missed or mixed declarations of ENTRY and
| KPROBES. These macros would help a bit (at least I hope so).
|

...

Ingo, don't apply it -- found a nit. Will fix.

- Cyrill -

Could we at least try this out in -next before we decide to make this X86 only?
I am aware that binutils can be a bit fragile but -next testing should make a good check on this.

Sam

[Sam Ravnborg - Sun, Nov 23, 2008 at 06:51:25PM +0100]

| On Sun, Nov 23, 2008 at 07:57:11PM +0300, Cyrill Gorcunov wrote:
| > It's usefull to catch unbalanced, missed or mixed declarations of ENTRY and
| > KPROBES. These macros would help a bit (at least I hope so).
| > 
| > For example the following code would compile without problems
| > 
| >         ENTRY_X86(mcount)
| >                 retq
| >         END_X86(mcount)
| > 
| > But if you forget and mix the following form
| > 
| >         ENTRY_X86(mcount)
| >                 retq
| >         END(mcount)
| > 
| >         ENTRY_X86(ftrace_caller)
| > 
| > The assembler will issue the following message:
| > Error: ENTRY_X86/KPROBE_X86 unbalanced,missed,mixed
| > 
| > Actually the checking is performed at every _X86 macro
| > so maybe it's good idea to put ENTRY_KPROBE_FINAL_X86
| > at the end of .S file to be sure you didn't miss anything.
| 
| Could we at least try this out in -next before we decide to make 
| this X86 only?
| I am aware that binutils can be a bit fragile but -next testing should
| make a good check on this.
| 
| 	Sam
| 

I don't have -next tree on my laptop, neither cross-compile tools but if someone could test it -- it would be great. But I used gas macros here -- i doubt other architectures has the same syntax. At least PDP-11 would beat us with ';' symbol :)

- Cyrill -

[Cyrill Gorcunov - Sun, Nov 23, 2008 at 08:58:46PM +0300]

| [Sam Ravnborg - Sun, Nov 23, 2008 at 06:51:25PM +0100]
| | On Sun, Nov 23, 2008 at 07:57:11PM +0300, Cyrill Gorcunov wrote:
| | > It's usefull to catch unbalanced, missed or mixed declarations of ENTRY and
| | > KPROBES. These macros would help a bit (at least I hope so).
| | > 
| | > For example the following code would compile without problems
| | > 
| | >         ENTRY_X86(mcount)
| | >                 retq
| | >         END_X86(mcount)
| | > 
| | > But if you forget and mix the following form
| | > 
| | >         ENTRY_X86(mcount)
| | >                 retq
| | >         END(mcount)
| | > 
| | >         ENTRY_X86(ftrace_caller)
| | > 
| | > The assembler will issue the following message:
| | > Error: ENTRY_X86/KPROBE_X86 unbalanced,missed,mixed
| | > 
| | > Actually the checking is performed at every _X86 macro
| | > so maybe it's good idea to put ENTRY_KPROBE_FINAL_X86
| | > at the end of .S file to be sure you didn't miss anything.
| | 
| | Could we at least try this out in -next before we decide to make 
| | this X86 only?
| | I am aware that binutils can be a bit fragile but -next testing should
| | make a good check on this.
| | 
| | 	Sam
| | 
| 
| I don't have -next tree on my laptop, neither cross-compile tools but
| if someone could test it -- it would be great. But I used gas macros
| here -- i doubt other architectures has the same syntax. At least
| PDP-11 would beat us with ';' symbol :)
| 
| 		- Cyrill -

On the other hand, if this feature show 'good' behaviour on x86 we could propagate it on other arch's. If we just turn it on by default -- lots of errors will be.

- Cyrill -

If we include this in any of the 100+ trees that Stephen sucks into -next we will get it tried out.

Ingo has so and so does others so getting it into -next is rather easy. Then the automated builds will tell of if it fails on any of the toolchains used there.

Sam

[Sam Ravnborg - Sun, Nov 23, 2008 at 07:12:48PM +0100]
...

| > 
| > I don't have -next tree on my laptop, neither cross-compile tools but
| > if someone could test it -- it would be great. But I used gas macros
| > here -- i doubt other architectures has the same syntax. At least
| > PDP-11 would beat us with ';' symbol :)
| 
| If we include this in any of the 100+ trees that Stephen sucks
| into -next we will get it tried out.
| 
| Ingo has so and so does others so getting it into -next
| is rather easy. Then the automated builds will tell of if
| it fails on any of the toolchains used there.
| 
| 	Sam
| 

Sam, to be clear, you mean that I could put this stuff into general include/linux/linkage.h with general names as ENTRY/END and the same for KPROBE so it could be merged into -next tree for testing? If yes, that as I said there will be a lot of errors so build will stuck in a moment 'cause of unbalanced ENTRY. Not sure if it's a good idea :)

- Cyrill -

[Cyrill Gorcunov - Sun, Nov 23, 2008 at 09:21:03PM +0300]

| [Sam Ravnborg - Sun, Nov 23, 2008 at 07:12:48PM +0100]
| ...
| | > 
| | > I don't have -next tree on my laptop, neither cross-compile tools but
| | > if someone could test it -- it would be great. But I used gas macros
| | > here -- i doubt other architectures has the same syntax. At least
| | > PDP-11 would beat us with ';' symbol :)
| | 
| | If we include this in any of the 100+ trees that Stephen sucks
| | into -next we will get it tried out.
| | 
| | Ingo has so and so does others so getting it into -next
| | is rather easy. Then the automated builds will tell of if
| | it fails on any of the toolchains used there.
| | 
| | 	Sam
| | 
| 
| Sam, to be clear, you mean that I could put this stuff into general
| include/linux/linkage.h with general names as ENTRY/END and the same
| for KPROBE so it could be merged into -next tree for testing? If yes,
| that as I said there will be a lot of errors so build will stuck in
| a moment 'cause of unbalanced ENTRY. Not sure if it's a good idea :)
| 
| 		- Cyrill -

Did I understand you right? It's highly possible I could translate message the wrong way somime.

- Cyrill -

Hi Cyrill,

Aborting might be a bit to heavyhanded for -next. Maybe you should prepare a version that only gives a warning for the first problem it encounters per file? That would be fine in -next.

Greetings,
Alexander

neither do i think it's a particularly good idea. Lets first prototype it on x86, see how it works out in practice, and then see whether it can be generic. Then it can just be lifted into the generic linkage.h separately, and we can then see whether it causes new problems.

Ingo

[Ingo Molnar - Sun, Nov 23, 2008 at 07:54:17PM +0100]

| 
| * Cyrill Gorcunov <[email protected]> wrote:
| 
| > [Sam Ravnborg - Sun, Nov 23, 2008 at 07:12:48PM +0100]
| > ...
| > | > 
| > | > I don't have -next tree on my laptop, neither cross-compile tools but
| > | > if someone could test it -- it would be great. But I used gas macros
| > | > here -- i doubt other architectures has the same syntax. At least
| > | > PDP-11 would beat us with ';' symbol :)
| > | 
| > | If we include this in any of the 100+ trees that Stephen sucks
| > | into -next we will get it tried out.
| > | 
| > | Ingo has so and so does others so getting it into -next
| > | is rather easy. Then the automated builds will tell of if
| > | it fails on any of the toolchains used there.
| > | 
| > | 	Sam
| > | 
| > 
| > Sam, to be clear, you mean that I could put this stuff into general 
| > include/linux/linkage.h with general names as ENTRY/END and the same 
| > for KPROBE so it could be merged into -next tree for testing? If 
| > yes, that as I said there will be a lot of errors so build will 
| > stuck in a moment 'cause of unbalanced ENTRY. Not sure if it's a 
| > good idea :)
| 
| neither do i think it's a particularly good idea. Lets first prototype 
| it on x86, see how it works out in practice, and then see whether it 
| can be generic. Then it can just be lifted into the generic linkage.h 
| separately, and we can then see whether it causes new problems. 
| 
| 	Ingo
| 

So be it :) Btw I think Alexander is right -- better to use .warning instead of .error (and without .abort) even on x86. Could you update Ingo?

- Cyrill -

.error is perfectly fine because that way automated tests that we do on -tip will catch any bugs, we really dont want to mis-annotate these things. Warnings tend to only pile up and rarely get fixed - without enforcement mechanism that causes people to fix them.

Ingo

[Ingo Molnar - Sun, Nov 23, 2008 at 08:00:25PM +0100]
...

| > 
| > So be it :) Btw I think Alexander is right -- better to use .warning 
| > instead of .error (and without .abort) even on x86. Could you update 
| > Ingo?
| 
| .error is perfectly fine because that way automated tests that we do 
| on -tip will catch any bugs, we really dont want to mis-annotate these 
| things. Warnings tend to only pile up and rarely get fixed - without 
| enforcement mechanism that causes people to fix them.
| 
| 	Ingo
| 

ok, let me try to play with this feature a bit. Will report the results.

- Cyrill -

[Ingo Molnar - Sun, Nov 23, 2008 at 08:00:25PM +0100]

| 
| * Cyrill Gorcunov <[email protected]> wrote:
| 
| > [Ingo Molnar - Sun, Nov 23, 2008 at 07:54:17PM +0100]
| > | 
| > | * Cyrill Gorcunov <[email protected]> wrote:
| > | 
| > | > [Sam Ravnborg - Sun, Nov 23, 2008 at 07:12:48PM +0100]
| > | > ...
| > | > | > 
| > | > | > I don't have -next tree on my laptop, neither cross-compile tools but
| > | > | > if someone could test it -- it would be great. But I used gas macros
| > | > | > here -- i doubt other architectures has the same syntax. At least
| > | > | > PDP-11 would beat us with ';' symbol :)
| > | > | 
| > | > | If we include this in any of the 100+ trees that Stephen sucks
| > | > | into -next we will get it tried out.
| > | > | 
| > | > | Ingo has so and so does others so getting it into -next
| > | > | is rather easy. Then the automated builds will tell of if
| > | > | it fails on any of the toolchains used there.
| > | > | 
| > | > | 	Sam
| > | > | 
| > | > 
| > | > Sam, to be clear, you mean that I could put this stuff into general 
| > | > include/linux/linkage.h with general names as ENTRY/END and the same 
| > | > for KPROBE so it could be merged into -next tree for testing? If 
| > | > yes, that as I said there will be a lot of errors so build will 
| > | > stuck in a moment 'cause of unbalanced ENTRY. Not sure if it's a 
| > | > good idea :)
| > | 
| > | neither do i think it's a particularly good idea. Lets first prototype 
| > | it on x86, see how it works out in practice, and then see whether it 
| > | can be generic. Then it can just be lifted into the generic linkage.h 
| > | separately, and we can then see whether it causes new problems. 
| > | 
| > | 	Ingo
| > | 
| > 
| > So be it :) Btw I think Alexander is right -- better to use .warning 
| > instead of .error (and without .abort) even on x86. Could you update 
| > Ingo?
| 
| .error is perfectly fine because that way automated tests that we do 
| on -tip will catch any bugs, we really dont want to mis-annotate these 
| things. Warnings tend to only pile up and rarely get fixed - without 
| enforcement mechanism that causes people to fix them.
| 
| 	Ingo
|

Just got an error in implementation -- we have to support nested ENTRY without problem. Will check. What a surprise :-)

- Cyrill -

do you mean:

ENTRY(system_call)
ENTRY(system_call_after_swapgs)
...
END(system_call)

that's more of a bug - system_call_after_swapgs is not a real entry point, we just need the label of it. Perhaps something like __ENTRY() for that case would be enough.

nor is this one real:

ENTRY(interrupt)
ENTRY(irq_entries_start)
...
END(irq_entries_start)
END(interrupt)

do we really need .irq_entries_start?

I think in general we should define a flat hierarchy of entries.

Ingo

[Ingo Molnar - Sun, Nov 23, 2008 at 08:31:34PM +0100]
...

| > 
| > Just got an error in implementation -- we have to support nested
| > ENTRY without problem. Will check. What a surprise :-)
| 
| do you mean:
| 
|   ENTRY(system_call)
|   ENTRY(system_call_after_swapgs)
|   ...
|   END(system_call)
| 
| that's more of a bug - system_call_after_swapgs is not a real entry 
| point, we just need the label of it. Perhaps something like __ENTRY() 
| for that case would be enough.
| 
| nor is this one real:
| 
|   ENTRY(interrupt)
|   ENTRY(irq_entries_start)
|   ...
|   END(irq_entries_start)
|   END(interrupt)
| 
| do we really need .irq_entries_start?
| 
| I think in general we should define a flat hierarchy of entries.
| 
| 	Ingo
| 

Yeah, I meant these cases. I don't think we really need irq_entries_start (didn't find any mention of them in tree). In case of system_call_after_swapgs I'm not that sure, but since xen use it as a plain jmp (at least now) it could be converted to a plain label. Ingo, I'll continue tomorrow evening -- have some other things to be done :)

- Cyrill -

OK - I assume you guys will take action on this if we succeed in x86 with this nice build time check.

Sam

system_call_after_swapgs is a slowpath and should be converted to a simple:

.globl system_call_after_swapgs
system_call_after_swapgs:

symbol definition - with no particular jump target alignment tweaks.

(the above sequence should be generalized as an __ENTRY() macro - i.e. raw global symbol definition without any alignment tweaks)

Ingo

The problem is that ENTRY(interrupt) is done in init.rodata, and ENTRY(irq_entries_start) is done in .text. So inside the .S-file, they are nested, but in the .o-file they are separate. Instead of removing ENTRY(irq_entries_start), I think we should just expand to:

	.section .init.rodata,"a"
	.p2align 5
	.global interrupt

interrupt:

and

size interrupt, .-interrupt

But the only importance I can think of is that this keeps both the "interrupt" array and irq_entries_start visible in debugging information.

Alternatively, we could probably do away with the interrupt array entirely. We _know_ how the irq stubs are structured and irq_entries_start is in principle enough information to reconstruct all information in interrupt.

Anyhow, I too think that supporting nested ENTRY/END in .S files is unnecessary.

Alexander

I'd rather not get rid of the interrupt array. But more fundamentally, interrupt is a data symbol, not an entry point.

-hpa