swsusp.S: meaningfull assembly labels

swsusp.S: meaningfull assembly labels

[Pavel Machek]

Hi!

This introduces meaningfull labels instead of .L1234, meaning code is
readable, kills alignment where unneccessary, and kills TLB flush that
was only pure paranoia (and slows it down a lot on emulated
systems). Please apply,

								Pavel

--- linux-cvs//arch/i386/power/swsusp.S	2004-05-25 17:41:18.000000000 +0200
+++ linux/arch/i386/power/swsusp.S	2004-06-24 14:39:01.000000000 +0200
@@ -18,7 +18,7 @@
 ENTRY(do_magic)
 	pushl %ebx
 	cmpl $0,8(%esp)
-	jne .L1450
+	jne resume
 	call do_magic_suspend_1
 	call save_processor_state
 
@@ -33,21 +33,21 @@
 	pushfl ; popl saved_context_eflags
 
 	call do_magic_suspend_2
-	jmp .L1449
-	.p2align 4,,7
-.L1450:
+	popl %ebx
+	ret
+
+resume:
 	movl $swsusp_pg_dir-__PAGE_OFFSET,%ecx
 	movl %ecx,%cr3
 
 	call do_magic_resume_1
 	movl $0,loop
 	cmpl $0,nr_copy_pages
-	je .L1453
-	.p2align 4,,7
-.L1455:
+	je copy_done
+copy_loop:
 	movl $0,loop2
 	.p2align 4,,7
-.L1459:
+copy_one_page:
 	movl pagedir_nosave,%ecx
 	movl loop,%eax
 	movl loop2,%edx
@@ -56,23 +56,21 @@
 	movl (%ecx,%eax),%eax
 	movb (%edx,%eax),%al
 	movb %al,(%edx,%ebx)
-	movl %cr3, %eax;              
-	movl %eax, %cr3;  # flush TLB 
 
 	movl loop2,%eax
 	leal 1(%eax),%edx
 	movl %edx,loop2
 	movl %edx,%eax
 	cmpl $4095,%eax
-	jbe .L1459
+	jbe copy_one_page
 	movl loop,%eax
 	leal 1(%eax),%edx
 	movl %edx,loop
 	movl %edx,%eax
 	cmpl nr_copy_pages,%eax
-	jb .L1455
-	.p2align 4,,7
-.L1453:
+	jb copy_loop
+
+copy_done:
 	movl $__USER_DS,%eax
 
 	movw %ax, %ds
@@ -88,7 +86,6 @@
 	call restore_processor_state
 	pushl saved_context_eflags ; popfl
 	call do_magic_resume_2
-.L1449:
 	popl %ebx
 	ret
 

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: swsusp.S: meaningfull assembly labels

[Richard B. Johnson]

On Fri, 25 Jun 2004, Pavel Machek wrote:

> Hi!
>
> This introduces meaningfull labels instead of .L1234, meaning code is
> readable, kills alignment where unneccessary, and kills TLB flush that
> was only pure paranoia (and slows it down a lot on emulated
> systems). Please apply,
>
> 								Pavel
>
> --- linux-cvs//arch/i386/power/swsusp.S	2004-05-25 17:41:18.000000000 +0200
> +++ linux/arch/i386/power/swsusp.S	2004-06-24 14:39:01.000000000 +0200
> @@ -18,7 +18,7 @@
>  ENTRY(do_magic)
>  	pushl %ebx
>  	cmpl $0,8(%esp)
> -	jne .L1450
> +	jne resume
>  	call do_magic_suspend_1
>  	call save_processor_state
>
> @@ -33,21 +33,21 @@
>  	pushfl ; popl saved_context_eflags
>
>  	call do_magic_suspend_2
> -	jmp .L1449
> -	.p2align 4,,7
> -.L1450:
> +	popl %ebx
> +	ret
> +
> +resume:
>  	movl $swsusp_pg_dir-__PAGE_OFFSET,%ecx
>  	movl %ecx,%cr3
>
>  	call do_magic_resume_1
>  	movl $0,loop
>  	cmpl $0,nr_copy_pages
> -	je .L1453
> -	.p2align 4,,7
> -.L1455:
> +	je copy_done
> +copy_loop:
>  	movl $0,loop2
>  	.p2align 4,,7
> -.L1459:
> +copy_one_page:
>  	movl pagedir_nosave,%ecx
>  	movl loop,%eax
>  	movl loop2,%edx
> @@ -56,23 +56,21 @@
>  	movl (%ecx,%eax),%eax
>  	movb (%edx,%eax),%al
>  	movb %al,(%edx,%ebx)
> -	movl %cr3, %eax;
> -	movl %eax, %cr3;  # flush TLB
>
>  	movl loop2,%eax
>  	leal 1(%eax),%edx
>  	movl %edx,loop2
>  	movl %edx,%eax
>  	cmpl $4095,%eax
> -	jbe .L1459
> +	jbe copy_one_page
>  	movl loop,%eax
>  	leal 1(%eax),%edx
>  	movl %edx,loop
>  	movl %edx,%eax
>  	cmpl nr_copy_pages,%eax
> -	jb .L1455
> -	.p2align 4,,7
> -.L1453:
> +	jb copy_loop
> +
> +copy_done:
>  	movl $__USER_DS,%eax
>
>  	movw %ax, %ds
> @@ -88,7 +86,6 @@
>  	call restore_processor_state
>  	pushl saved_context_eflags ; popfl
>  	call do_magic_resume_2
> -.L1449:
>  	popl %ebx
>  	ret
>
>
> --

NO! You just made those labels public! The LOCAL symbols need to
begin with ".L".  Now, if you have a 'copy_loop' in another module,
linked with this, anywhere in the kernel, they will share the
same address -- not what you expected, I'm sure! The assembler
has some strange rules you need to understand. Use `nm` and you
will find that your new labels are in the object file!

Cheers,
Dick Johnson
Penguin : Linux version 2.4.26 on an i686 machine (5570.56 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: swsusp.S: meaningfull assembly labels

[Daniel Jacobowitz]

On Fri, Jun 25, 2004 at 08:37:45AM -0400, Richard B. Johnson wrote:
> NO! You just made those labels public! The LOCAL symbols need to
> begin with ".L".  Now, if you have a 'copy_loop' in another module,
> linked with this, anywhere in the kernel, they will share the
> same address -- not what you expected, I'm sure! The assembler
> has some strange rules you need to understand. Use `nm` and you
> will find that your new labels are in the object file!

Er, no.  They'll show up in the object file.  That doesn't mean they're
global; static symbols also show up in the object file.

-- 
Daniel Jacobowitz

Re: swsusp.S: meaningfull assembly labels

[Richard B. Johnson]

On Fri, 25 Jun 2004, Daniel Jacobowitz wrote:

> On Fri, Jun 25, 2004 at 08:37:45AM -0400, Richard B. Johnson wrote:
> > NO! You just made those labels public! The LOCAL symbols need to
> > begin with ".L".  Now, if you have a 'copy_loop' in another module,
> > linked with this, anywhere in the kernel, they will share the
> > same address -- not what you expected, I'm sure! The assembler
> > has some strange rules you need to understand. Use `nm` and you
> > will find that your new labels are in the object file!
>
> Er, no.  They'll show up in the object file.  That doesn't mean they're
> global; static symbols also show up in the object file.
>
> --
> Daniel Jacobowitz

I got caught on these, thinking that they weren't globals when
I made a assembly files that used, not only named labels but
also definitions like:

BUF = 0x08
LEN = 0x0c

code:	movl	BUF(%esp), %ebx
	movl	LEN(%esp), %ecx


This was done in several files. When linked, I got 'duplicate synbol'
errors. These symbols, while not 'globals' in the sense that 'C'
code can link with them are global definitions that are seen by
the linker and cause duplicate symbol errors. I went through
the gas documentation, trying to find how to prevent these
private symbols from being written to the object file and there
is no command-line mechanism. You just have to start every name
with .L to keep them local.

When I used labels like 'code' above, even though not declared
'.global',  the labels also showed up as duplicate symbols when
linking the resulting object files.


Cheers,
Dick Johnson
Penguin : Linux version 2.4.26 on an i686 machine (5570.56 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: swsusp.S: meaningfull assembly labels

[Pavel Machek]

Hi!

> > This introduces meaningfull labels instead of .L1234, meaning code is
> > readable, kills alignment where unneccessary, and kills TLB flush that
> > was only pure paranoia (and slows it down a lot on emulated
> > systems). Please apply,
> >
> > 								Pavel
> >
> > --- linux-cvs//arch/i386/power/swsusp.S	2004-05-25 17:41:18.000000000 +0200
> > +++ linux/arch/i386/power/swsusp.S	2004-06-24 14:39:01.000000000 +0200
> > @@ -18,7 +18,7 @@
> >  ENTRY(do_magic)
> >  	pushl %ebx
> >  	cmpl $0,8(%esp)
> > -	jne .L1450
> > +	jne resume
> >  	call do_magic_suspend_1
> >  	call save_processor_state
> >
...
> NO! You just made those labels public! The LOCAL symbols need to
> begin with ".L".  Now, if you have a 'copy_loop' in another module,
> linked with this, anywhere in the kernel, they will share the
> same address -- not what you expected, I'm sure! The assembler
> has some strange rules you need to understand. Use `nm` and you
> will find that your new labels are in the object file!

Are you sure? I thought theare not visible from other moduless unless
"ENTRY()" is used.  See for example entry.S.
							Pavel

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: swsusp.S: meaningfull assembly labels

[Richard B. Johnson]

On Fri, 25 Jun 2004, Pavel Machek wrote:

> Hi!
>
> > > This introduces meaningfull labels instead of .L1234, meaning code is
> > > readable, kills alignment where unneccessary, and kills TLB flush that
> > > was only pure paranoia (and slows it down a lot on emulated
> > > systems). Please apply,
> > >
> > > 								Pavel
> > >
> > > --- linux-cvs//arch/i386/power/swsusp.S	2004-05-25 17:41:18.000000000 +0200
> > > +++ linux/arch/i386/power/swsusp.S	2004-06-24 14:39:01.000000000 +0200
> > > @@ -18,7 +18,7 @@
> > >  ENTRY(do_magic)
> > >  	pushl %ebx
> > >  	cmpl $0,8(%esp)
> > > -	jne .L1450
> > > +	jne resume
> > >  	call do_magic_suspend_1
> > >  	call save_processor_state
> > >
> ...
> > NO! You just made those labels public! The LOCAL symbols need to
> > begin with ".L".  Now, if you have a 'copy_loop' in another module,
> > linked with this, anywhere in the kernel, they will share the
> > same address -- not what you expected, I'm sure! The assembler
> > has some strange rules you need to understand. Use `nm` and you
> > will find that your new labels are in the object file!
>
> Are you sure? I thought theare not visible from other moduless unless
> "ENTRY()" is used.  See for example entry.S.
> 							Pavel

Well if you find a way to make those symbols disappear from the
object files without using ".L", please let me know. I spent
some considerable time beautifying multiple assembly-language
files on a recent project, only to find that it would fail to
link because of the "multiple symbol definition" errors. I
had to change everything back.

You can make labels like ".Lresume:" remain visible only in
the file. They don't need to be numbers. It may even be possible
to use labels like ".L_resume:", I haven't tried. Basically, if
they show up using `nm`, they can (read will) give you problems.


Cheers,
Dick Johnson
Penguin : Linux version 2.4.26 on an i686 machine (5570.56 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: swsusp.S: meaningfull assembly labels

[Daniel Jacobowitz]

On Fri, Jun 25, 2004 at 12:02:47PM -0400, Richard B. Johnson wrote:
> On Fri, 25 Jun 2004, Daniel Jacobowitz wrote:
> 
> > On Fri, Jun 25, 2004 at 08:37:45AM -0400, Richard B. Johnson wrote:
> > > NO! You just made those labels public! The LOCAL symbols need to
> > > begin with ".L".  Now, if you have a 'copy_loop' in another module,
> > > linked with this, anywhere in the kernel, they will share the
> > > same address -- not what you expected, I'm sure! The assembler
> > > has some strange rules you need to understand. Use `nm` and you
> > > will find that your new labels are in the object file!
> >
> > Er, no.  They'll show up in the object file.  That doesn't mean they're
> > global; static symbols also show up in the object file.
> >
> > --
> > Daniel Jacobowitz
> 
> I got caught on these, thinking that they weren't globals when
> I made a assembly files that used, not only named labels but
> also definitions like:
> 
> BUF = 0x08
> LEN = 0x0c
> 
> code:	movl	BUF(%esp), %ebx
> 	movl	LEN(%esp), %ecx
> 
> 
> This was done in several files. When linked, I got 'duplicate synbol'
> errors. These symbols, while not 'globals' in the sense that 'C'
> code can link with them are global definitions that are seen by
> the linker and cause duplicate symbol errors. I went through

Those are what gas calls absolute symbols.

> the gas documentation, trying to find how to prevent these
> private symbols from being written to the object file and there
> is no command-line mechanism. You just have to start every name
> with .L to keep them local.
> 
> When I used labels like 'code' above, even though not declared
> '.global',  the labels also showed up as duplicate symbols when
> linking the resulting object files.

Does this only happen in your world?  Or were you using a broken
non-GNU linker?

drow@nevyn:~% cat a.s
BUF = 0x1
code:
        mov %eax, 0
drow@nevyn:~% as -o a.o a.s  
drow@nevyn:~% as -o b.o a.s
drow@nevyn:~% ld -o a a.o b.o
ld: warning: cannot find entry symbol _start; defaulting to 0000000008048074
drow@nevyn:~% nm a.o
00000001 a BUF
00000000 t code
drow@nevyn:~% nm a
00000001 a BUF
00000001 a BUF
08049084 A __bss_start
08049084 A _edata
08049084 A _end
08048074 t code
0804807c t code

LD won't even warn about duplicate absolute symbols unless they have
different values.

-- 
Daniel Jacobowitz

Re: swsusp.S: meaningfull assembly labels

[Richard B. Johnson]

On Fri, 25 Jun 2004, Daniel Jacobowitz wrote:

> On Fri, Jun 25, 2004 at 12:02:47PM -0400, Richard B. Johnson wrote:
> > On Fri, 25 Jun 2004, Daniel Jacobowitz wrote:
> >
> > > On Fri, Jun 25, 2004 at 08:37:45AM -0400, Richard B. Johnson wrote:
> > > > NO! You just made those labels public! The LOCAL symbols need to
> > > > begin with ".L".  Now, if you have a 'copy_loop' in another module,
> > > > linked with this, anywhere in the kernel, they will share the
> > > > same address -- not what you expected, I'm sure! The assembler
> > > > has some strange rules you need to understand. Use `nm` and you
> > > > will find that your new labels are in the object file!
> > >
> > > Er, no.  They'll show up in the object file.  That doesn't mean they're
> > > global; static symbols also show up in the object file.
> > >
> > > --
> > > Daniel Jacobowitz
> >
> > I got caught on these, thinking that they weren't globals when
> > I made a assembly files that used, not only named labels but
> > also definitions like:
> >
> > BUF = 0x08
> > LEN = 0x0c
> >
> > code:	movl	BUF(%esp), %ebx
> > 	movl	LEN(%esp), %ecx
> >
> >
> > This was done in several files. When linked, I got 'duplicate synbol'
> > errors. These symbols, while not 'globals' in the sense that 'C'
> > code can link with them are global definitions that are seen by
> > the linker and cause duplicate symbol errors. I went through
>
> Those are what gas calls absolute symbols.
>
> > the gas documentation, trying to find how to prevent these
> > private symbols from being written to the object file and there
> > is no command-line mechanism. You just have to start every name
> > with .L to keep them local.
> >
> > When I used labels like 'code' above, even though not declared
> > '.global',  the labels also showed up as duplicate symbols when
> > linking the resulting object files.
>
> Does this only happen in your world?  Or were you using a broken
> non-GNU linker?
>

It is the GNU linker

> drow@nevyn:~% cat a.s
> BUF = 0x1
> code:
>         mov %eax, 0
> drow@nevyn:~% as -o a.o a.s
> drow@nevyn:~% as -o b.o a.s
> drow@nevyn:~% ld -o a a.o b.o
> ld: warning: cannot find entry symbol _start; defaulting to 0000000008048074
> drow@nevyn:~% nm a.o
> 00000001 a BUF
> 00000000 t code
> drow@nevyn:~% nm a
> 00000001 a BUF
> 00000001 a BUF
> 08049084 A __bss_start
> 08049084 A _edata
> 08049084 A _end
> 08048074 t code
> 0804807c t code
>
> LD won't even warn about duplicate absolute symbols unless they have
> different values.
>

But they ARE different values. They will probably always be different
values, and they end up with the same name, that `ld` sees and complains.


> --
> Daniel Jacobowitz
>

Cheers,
Dick Johnson
Penguin : Linux version 2.4.26 on an i686 machine (5570.56 BogoMips).
            Note 96.31% of all statistics are fiction.

Re: swsusp.S: meaningfull assembly labels

[Patrick Mochel]

> > drow@nevyn:~% cat a.s
> > BUF = 0x1
> > code:
> >         mov %eax, 0
> > drow@nevyn:~% as -o a.o a.s
> > drow@nevyn:~% as -o b.o a.s
> > drow@nevyn:~% ld -o a a.o b.o
> > ld: warning: cannot find entry symbol _start; defaulting to 0000000008048074
> > drow@nevyn:~% nm a.o
> > 00000001 a BUF
> > 00000000 t code
> > drow@nevyn:~% nm a
> > 00000001 a BUF
> > 00000001 a BUF
> > 08049084 A __bss_start
> > 08049084 A _edata
> > 08049084 A _end
> > 08048074 t code
> > 0804807c t code
> >
> > LD won't even warn about duplicate absolute symbols unless they have
> > different values.
> >
>
> But they ARE different values. They will probably always be different
> values, and they end up with the same name, that `ld` sees and complains.

No, it doesn't complain, that's the point. According to nm(1):

       For each symbol, nm shows:

          The symbol value, in the  radix  selected  by  options
           (see below), or hexadecimal by default.

          The  symbol  type.   At  least the following types are
           used; others are, as well,  depending  on  the  object
           file  format.   If  lowercase, the symbol is local; if
           uppercase, the symbol is global (external).

Symbols in a source file that are static are tagged with a 't' as well.
Read System.map for more proof of this.

The patch is fine, especially since I think it's based on something I've
posted in the past. :) The one thing I'd always done differently, though,
was to prefix the labels with '.', since I thought that was what kept it
local. But, after I RTFM'd, I find that's not even necessary.


	Pat