Re: STT_FUNC for assembler checksum and semaphore ops" in git-x86
Andi Kleen wrote: > On Thu, Jan 10, 2008 at 04:59:52PM -0800, John Reiser wrote: > >>Andi Kleen wrote: >> >>>But actually checking the default implementation in linkage.h already >>>implements size: [snip] >> >>>Are you sure it doesn't work? Your patch should be not needed. If it's >>>still wrong then just ENDPROCs() need to be added. >> >>The ENDPROCs() were not used everywhere. Some code used just END() instead, >>while other code used nothing. um/sys-i386/checksum.S didn't #include > > > END() is fine too since it contains .size too: > > #ifndef END > #define END(name) \ > .size name, .-name > #endif > > >>diff --git a/arch/x86/lib/semaphore_32.S b/arch/x86/lib/semaphore_32.S >>index 444fba4..e2c6e0d 100644 >>--- a/arch/x86/lib/semaphore_32.S >>+++ b/arch/x86/lib/semaphore_32.S >>@@ -49,7 +49,7 @@ ENTRY(__down_failed) >> ENDFRAME >> ret >> CFI_ENDPROC >>- END(__down_failed) >>+ ENDPROC(__down_failed) > > > I don't think these change makes sense given the definition of END() > shown above. > > The only change that would make sense is adding END() (or ENDPROC()) > to a function that doesn't have either of them yet. No. The pseudo op ".type name, @function" appears only in ENDPROC; it does not appear in END. So changing END to ENDPROC *does* alter the Elf32_Sym for 'name'. Just END produces STT_NOTYPE; ENDPROC produces STT_FUNC. A static analysis tool can get the info it wants much more easily if all subroutines are marked as STT_FUNC. In theory the tool could sort the symbols, notice the disjoint coverage of the address space by the .size intervals of consecutive symbols that are the targets of a CALL instruction, and thus deduce that ".type foo, @function" *should* have been specified. But this is a heuristic, and it fails on boundaries where assembly code is invoked via trap, interrupt, or exception (anything other than CALL). Instead, specify STT_FUNC for each subroutine in the first place. That requires .type, which means ENDPROC (not END) from linux/linkage.h. -- John Reiser, [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: STT_FUNC for assembler checksum and semaphore ops" in git-x86
On Thu, Jan 10, 2008 at 04:59:52PM -0800, John Reiser wrote: > Andi Kleen wrote: > > But actually checking the default implementation in linkage.h already > > implements size: [snip] > > > Are you sure it doesn't work? Your patch should be not needed. If it's > > still wrong then just ENDPROCs() need to be added. > > The ENDPROCs() were not used everywhere. Some code used just END() instead, > while other code used nothing. um/sys-i386/checksum.S didn't #include END() is fine too since it contains .size too: #ifndef END #define END(name) \ .size name, .-name #endif > diff --git a/arch/x86/lib/semaphore_32.S b/arch/x86/lib/semaphore_32.S > index 444fba4..e2c6e0d 100644 > --- a/arch/x86/lib/semaphore_32.S > +++ b/arch/x86/lib/semaphore_32.S > @@ -49,7 +49,7 @@ ENTRY(__down_failed) > ENDFRAME > ret > CFI_ENDPROC > - END(__down_failed) > + ENDPROC(__down_failed) I don't think these change makes sense given the definition of END() shown above. The only change that would make sense is adding END() (or ENDPROC()) to a function that doesn't have either of them yet. Since you seem to do nop changes something is wrong with your testing procedure? -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: STT_FUNC for assembler checksum and semaphore ops" in git-x86
Andi Kleen wrote:
> But actually checking the default implementation in linkage.h already
> implements size: [snip]
> Are you sure it doesn't work? Your patch should be not needed. If it's
> still wrong then just ENDPROCs() need to be added.
The ENDPROCs() were not used everywhere. Some code used just END() instead,
while other code used nothing. um/sys-i386/checksum.S didn't #include
. I also got confused because gcc puts the
.type near the ENTRY, while ENDPROC puts it on the opposite end.
Here is a revised patch against linux-2.6-x86.git , including a comment
in linkage.h which explains one motivation. PowerPC is different
("_GLOBAL" instead of "ENTRY") and foreign to me, so I left it alone.
Signed off by: John Reiser <[EMAIL PROTECTED]>
diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index ff203dd..024cfab 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -53,6 +53,10 @@
.size name, .-name
#endif
+/* If symbol 'name' is treated as a subroutine (gets called, and returns)
+ * then please use ENDPROC to mark 'name' as STT_FUNC for the benefit of
+ * static analysis tools such as stack depth analyzer.
+ */
#ifndef ENDPROC
#define ENDPROC(name) \
.type name, @function; \
diff --git a/arch/x86/lib/semaphore_32.S b/arch/x86/lib/semaphore_32.S
index 444fba4..e2c6e0d 100644
--- a/arch/x86/lib/semaphore_32.S
+++ b/arch/x86/lib/semaphore_32.S
@@ -49,7 +49,7 @@ ENTRY(__down_failed)
ENDFRAME
ret
CFI_ENDPROC
- END(__down_failed)
+ ENDPROC(__down_failed)
ENTRY(__down_failed_interruptible)
CFI_STARTPROC
@@ -70,7 +70,7 @@ ENTRY(__down_failed_interruptible)
ENDFRAME
ret
CFI_ENDPROC
- END(__down_failed_interruptible)
+ ENDPROC(__down_failed_interruptible)
ENTRY(__down_failed_trylock)
CFI_STARTPROC
@@ -91,7 +91,7 @@ ENTRY(__down_failed_trylock)
ENDFRAME
ret
CFI_ENDPROC
- END(__down_failed_trylock)
+ ENDPROC(__down_failed_trylock)
ENTRY(__up_wakeup)
CFI_STARTPROC
@@ -112,7 +112,7 @@ ENTRY(__up_wakeup)
ENDFRAME
ret
CFI_ENDPROC
- END(__up_wakeup)
+ ENDPROC(__up_wakeup)
/*
* rw spinlock fallbacks
@@ -132,7 +132,7 @@ ENTRY(__write_lock_failed)
ENDFRAME
ret
CFI_ENDPROC
- END(__write_lock_failed)
+ ENDPROC(__write_lock_failed)
ENTRY(__read_lock_failed)
CFI_STARTPROC
@@ -148,7 +148,7 @@ ENTRY(__read_lock_failed)
ENDFRAME
ret
CFI_ENDPROC
- END(__read_lock_failed)
+ ENDPROC(__read_lock_failed)
#endif
@@ -170,7 +170,7 @@ ENTRY(call_rwsem_down_read_failed)
CFI_ADJUST_CFA_OFFSET -4
ret
CFI_ENDPROC
- END(call_rwsem_down_read_failed)
+ ENDPROC(call_rwsem_down_read_failed)
ENTRY(call_rwsem_down_write_failed)
CFI_STARTPROC
@@ -182,7 +182,7 @@ ENTRY(call_rwsem_down_write_failed)
CFI_ADJUST_CFA_OFFSET -4
ret
CFI_ENDPROC
- END(call_rwsem_down_write_failed)
+ ENDPROC(call_rwsem_down_write_failed)
ENTRY(call_rwsem_wake)
CFI_STARTPROC
@@ -196,7 +196,7 @@ ENTRY(call_rwsem_wake)
CFI_ADJUST_CFA_OFFSET -4
1: ret
CFI_ENDPROC
- END(call_rwsem_wake)
+ ENDPROC(call_rwsem_wake)
/* Fix up special calling conventions */
ENTRY(call_rwsem_downgrade_wake)
@@ -214,6 +214,6 @@ ENTRY(call_rwsem_downgrade_wake)
CFI_ADJUST_CFA_OFFSET -4
ret
CFI_ENDPROC
- END(call_rwsem_downgrade_wake)
+ ENDPROC(call_rwsem_downgrade_wake)
#endif
diff --git a/arch/um/sys-i386/checksum.S b/arch/um/sys-i386/checksum.S
index 62c7e56..4f3f62b 100644
--- a/arch/um/sys-i386/checksum.S
+++ b/arch/um/sys-i386/checksum.S
@@ -26,6 +26,7 @@
*/
#include
+#include
/*
* computes a partial checksum, e.g. for TCP/UDP fragments
@@ -48,7 +49,7 @@ unsigned int csum_partial(const unsigned char * buff, int
len, unsigned int sum)
* Fortunately, it is easy to convert 2-byte alignment to 4-byte
* alignment for the unrolled loop.
*/
-csum_partial:
+ENTRY(csum_partial)
pushl %esi
pushl %ebx
movl 20(%esp),%eax # Function arg: unsigned int sum
@@ -113,12 +114,13 @@ csum_partial:
popl %ebx
popl %esi
ret
+ ENDPROC(csum_partial)
#else
/* Version for PentiumII/PPro */
-csum_partial:
+ENTRY(csum_partial)
pushl %esi
pushl %ebx
movl 20(%esp),%eax # Function arg: unsigned int sum
@@ -211,6 +213,7 @@ csum_partial:
popl %ebx
popl %esi
ret
+ ENDPROC(csum_partial)
#endif
@@ -250,7 +253,7 @@ unsigned int csum_partial_copy_generic (const char *src,
char *dst,
#define ARGBASE 16
#define FP 12
-csum_partial_copy_generic_i386:
+ENTRY(csum_partial_copy_generic_i386)
Re: STT_FUNC for assembler checksum and semaphore ops" in git-x86
> It does look ugly. But .size and .type are oriented towards static > description, while the dwarf2 CurrentFrameInfo (CFI) annotations > are oriented towards dynamic traceback and unwinding. Forcing the ENTRY/ENDPROC have nothing to do with dwarf2. That is CFI_STARTPROC/ENDPROC. But actually checking the default implementation in linkage.h already implements size: #ifndef END #define END(name) \ .size name, .-name #endif #ifndef ENDPROC #define ENDPROC(name) \ .type name, @function; \ END(name) #endif Are you sure it doesn't work? Your patch should be not needed. If it's still wrong then just ENDPROCs() need to be added. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: STT_FUNC for assembler checksum and semaphore ops" in git-x86
>>+ .type csum_partial, @function >> ENTRY(csum_partial) >>+ .type csum_partial, @function >> ENTRY(csum_partial) >>CFI_STARTPROC >>pushl %esi >>@@ -141,11 +142,13 @@ ENTRY(csum_partial) >>ret >>CFI_ENDPROC >> ENDPROC(csum_partial) >>+ .size csum_partial, . - csum_partial >>AK [Andi Kleen]: >>Better option would be to just add to ENTRY/ENDPROC ... > John got more or less same comment from me [Sam Ravnborg] - but > I did not hear further. > As it stands out now it not nice. It does look ugly. But .size and .type are oriented towards static description, while the dwarf2 CurrentFrameInfo (CFI) annotations are oriented towards dynamic traceback and unwinding. Forcing the coupling between static and dynamic annotation might lead to trouble when the fast path to 'ret' is in the middle, and code for the slow path appears after the 'ret'; or when a recursive tail 'falls through" into the entry point. A quick test shows that multiple .size pseudo-ops for the same symbol are accepted (the last one wins) so default coupling can be overridden. I'll test revised macros and report shortly. -- John Reiser, [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: STT_FUNC for assembler checksum and semaphore ops" in git-x86
On Wed, Jan 09, 2008 at 10:57:25PM +0100, Andi Kleen wrote: > > In gitx86: > > commit 692effca950d7c6032e8e2ae785a32383e7af4a3 > Author: John Reiser <[EMAIL PROTECTED]> > Date: Wed Jan 9 13:31:12 2008 +0100 > > STT_FUNC for assembler checksum and semaphore ops > ... > Comments? > > Signed-off-by: Thomas Gleixner <[EMAIL PROTECTED]> > Signed-off-by: Ingo Molnar <[EMAIL PROTECTED]> > > diff --git a/arch/x86/lib/checksum_32.S b/arch/x86/lib/checksum_32.S > index adbccd0..1f9aacb 100644 > --- a/arch/x86/lib/checksum_32.S > +++ b/arch/x86/lib/checksum_32.S > @@ -48,6 +48,7 @@ unsigned int csum_partial(const unsigned char * buff, int > len, unsigned int sum) >* Fortunately, it is easy to convert 2-byte alignment to 4-byte >* alignment for the unrolled loop. >*/ > + .type csum_partial, @function > ENTRY(csum_partial) > + .type csum_partial, @function > ENTRY(csum_partial) > CFI_STARTPROC > pushl %esi > @@ -141,11 +142,13 @@ ENTRY(csum_partial) > ret > CFI_ENDPROC > ENDPROC(csum_partial) > + .size csum_partial, . - csum_partial > > AK: > Better option would be to just add to ENTRY/ENDPROC > > do something like (untested) > > #define ENTRY(x) \ > ... > .set curfunc, x > > #define ENDPROC(x) \ > ... > .size x - curfunc > John got more or less same comment from me - but I did not hear further. As it stands out now it not nice. Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
STT_FUNC for assembler checksum and semaphore ops" in git-x86
In gitx86: commit 692effca950d7c6032e8e2ae785a32383e7af4a3 Author: John Reiser <[EMAIL PROTECTED]> Date: Wed Jan 9 13:31:12 2008 +0100 STT_FUNC for assembler checksum and semaphore ops ... Comments? Signed-off-by: Thomas Gleixner <[EMAIL PROTECTED]> Signed-off-by: Ingo Molnar <[EMAIL PROTECTED]> diff --git a/arch/x86/lib/checksum_32.S b/arch/x86/lib/checksum_32.S index adbccd0..1f9aacb 100644 --- a/arch/x86/lib/checksum_32.S +++ b/arch/x86/lib/checksum_32.S @@ -48,6 +48,7 @@ unsigned int csum_partial(const unsigned char * buff, int len, unsigned int sum) * Fortunately, it is easy to convert 2-byte alignment to 4-byte * alignment for the unrolled loop. */ + .type csum_partial, @function ENTRY(csum_partial) + .type csum_partial, @function ENTRY(csum_partial) CFI_STARTPROC pushl %esi @@ -141,11 +142,13 @@ ENTRY(csum_partial) ret CFI_ENDPROC ENDPROC(csum_partial) + .size csum_partial, . - csum_partial AK: Better option would be to just add to ENTRY/ENDPROC do something like (untested) #define ENTRY(x) \ ... .set curfunc, x #define ENDPROC(x) \ ... .size x - curfunc -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
