[PATCH] Re: Gate DSO not building properly?

[PATCH] Re: Gate DSO not building properly?

[Ian Wienand]

[-- Attachment #1: Type: text/plain, Size: 823 bytes --]

On Tue, Oct 21, 2003 at 04:10:51PM +1000, Ian Wienand wrote:
> arch/ia64/kernel/gate-data.S:1: Warning: setting incorrect section
> attributes for .data.gate

I figured out why (sorry if it's obvious); gas matches anything with
prefix '.data.' as a data special section and flags it as 'SHF_ALLOC +
SHF_WRITE' as per ELF [*].  Issuing 

.section .data.gate, "ax"

thus conflicts as it's assumed to be an extension of a .data section.

Should this even be in data at all, considering it's code?  What about
something along the lines of the attached patch which puts it in it's
own section?

-i
ianw@gelato.unsw.edu.au
http://www.gelato.unsw.edu.au

[*] For those who don't know, the exact rules about what sections and
how prefixes get matched are in bfd/elf-bfd.h(struct
bfd_elf_special_section) and bfd/elf.c in binutils.

[-- Attachment #2: gate-wrapper.patch --]
[-- Type: text/plain, Size: 3724 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1356  -> 1.1358 
#	arch/ia64/kernel/gate-data.S	1.1     -> 1.3     arch/ia64/kernel/gate-wrapper.S (moved)
#	arch/ia64/kernel/vmlinux.lds.S	1.38    -> 1.40   
#	arch/ia64/kernel/Makefile	1.26    -> 1.27   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/10/22	ianw@lemon.gelato.unsw.edu.au	1.1357
# The kernel DSO should not be in a .data section, so put it in its own.
# --------------------------------------------
# 03/10/22	ianw@lemon.gelato.unsw.edu.au	1.1358
# Separate out .linux_gate section
# --------------------------------------------
#
diff -Nru a/arch/ia64/kernel/Makefile b/arch/ia64/kernel/Makefile
--- a/arch/ia64/kernel/Makefile	Wed Oct 22 13:37:42 2003
+++ b/arch/ia64/kernel/Makefile	Wed Oct 22 13:37:42 2003
@@ -4,7 +4,7 @@
 
 extra-y	:= head.o init_task.o vmlinux.lds.s
 
-obj-y := acpi.o entry.o efi.o efi_stub.o gate-data.o fsys.o ia64_ksyms.o irq.o irq_ia64.o	\
+obj-y := acpi.o entry.o efi.o efi_stub.o gate-wrapper.o fsys.o ia64_ksyms.o irq.o irq_ia64.o	\
 	 irq_lsapic.o ivt.o machvec.o pal.o patch.o process.o perfmon.o ptrace.o sal.o		\
 	 semaphore.o setup.o signal.o sys_ia64.o time.o traps.o unaligned.o unwind.o
 
@@ -41,7 +41,7 @@
 $(obj)/gate-syms.o: $(src)/gate.lds.s $(obj)/gate.o FORCE
 	$(call if_changed,gate)
 
-# gate-data.o contains the gate DSO image as data in section .data.gate.
+# gate-wrapper.o contains the gate DSO image in the section .linux_gate.
 # We must build gate.so before we can assemble it.
 # Note: kbuild does not track this dependency due to usage of .incbin
-$(obj)/gate-data.o: $(obj)/gate.so
+$(obj)/gate-wrapper.o: $(obj)/gate.so
diff -Nru a/arch/ia64/kernel/gate-data.S b/arch/ia64/kernel/gate-data.S
--- a/arch/ia64/kernel/gate-data.S	Wed Oct 22 13:37:42 2003
+++ /dev/null	Wed Dec 31 16:00:00 1969
@@ -1,3 +0,0 @@
-	.section .data.gate, "ax"
-
-	.incbin "arch/ia64/kernel/gate.so"
diff -Nru a/arch/ia64/kernel/gate-wrapper.S b/arch/ia64/kernel/gate-wrapper.S
--- /dev/null	Wed Dec 31 16:00:00 1969
+++ b/arch/ia64/kernel/gate-wrapper.S	Wed Oct 22 13:37:42 2003
@@ -0,0 +1,8 @@
+/*
+ * This creates gate-wrapper.o which is an 
+ * object file that gets put into vmlinux.
+ * All it contains is the gate DSO
+ */
+	.section .linux_gate, "ax"
+
+	.incbin "arch/ia64/kernel/gate.so"
diff -Nru a/arch/ia64/kernel/vmlinux.lds.S b/arch/ia64/kernel/vmlinux.lds.S
--- a/arch/ia64/kernel/vmlinux.lds.S	Wed Oct 22 13:37:42 2003
+++ b/arch/ia64/kernel/vmlinux.lds.S	Wed Oct 22 13:37:42 2003
@@ -161,17 +161,26 @@
   .data.init_task : AT(ADDR(.data.init_task) - LOAD_OFFSET)
 	{ *(.data.init_task) }
 
+  /* Page aligned data */
   .data.page_aligned : AT(ADDR(.data.page_aligned) - LOAD_OFFSET)
-        { *(__special_page_section)
-	  __start_gate_section = .;
-	  *(.data.gate)
-	  __stop_gate_section = .;
+        { 
+		*(__special_page_section)
+		*(.data.page_aligned)
 	}
-  . = ALIGN(PAGE_SIZE);		/* make sure the gate page doesn't expose kernel data */
-
+  . = ALIGN(PAGE_SIZE);
+	
   .data.cacheline_aligned : AT(ADDR(.data.cacheline_aligned) - LOAD_OFFSET)
         { *(.data.cacheline_aligned) }
 
+  /* The linux-gate.so shared object */
+  .linux_gate : AT(ADDR(.linux_gate) - LOAD_OFFSET)
+        { __start_gate_section = .;
+	  *(.linux_gate)
+	  __stop_gate_section = .;
+	}
+  . = ALIGN(PAGE_SIZE);		/* make sure the gate page doesn't expose kernel data */
+
+	
   /* Per-cpu data: */
   . = ALIGN(PERCPU_PAGE_SIZE);
   __phys_per_cpu_start = .;

Re: [PATCH] Re: Gate DSO not building properly?

[H. J. Lu]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

On Wed, Oct 22, 2003 at 01:45:47PM +1000, Ian Wienand wrote:
> On Tue, Oct 21, 2003 at 04:10:51PM +1000, Ian Wienand wrote:
> > arch/ia64/kernel/gate-data.S:1: Warning: setting incorrect section
> > attributes for .data.gate
> 
> I figured out why (sorry if it's obvious); gas matches anything with
> prefix '.data.' as a data special section and flags it as 'SHF_ALLOC +
> SHF_WRITE' as per ELF [*].  Issuing 
> 
> .section .data.gate, "ax"
> 
> thus conflicts as it's assumed to be an extension of a .data section.
> 

Here is a patch for gas. Gas should set the right section attributes
no matter what.


H.J.

[-- Attachment #2: gas-attr-1.patch --]
[-- Type: text/plain, Size: 948 bytes --]

2003-10-21  H.J. Lu  <hongjiu.lu@intel.com>

	* config/obj-elf.c (obj_elf_change_section): Use specified
	section attributes.

--- gas/config/obj-elf.c.attr	2003-09-23 08:16:44.000000000 -0700
+++ gas/config/obj-elf.c	2003-10-21 22:31:51.000000000 -0700
@@ -615,6 +615,7 @@ obj_elf_change_section (name, type, attr
   segT sec;
   flagword flags;
   const struct bfd_elf_special_section *ssect;
+  bfd_boolean override = FALSE;
 
 #ifdef md_flush_pending_output
   md_flush_pending_output ();
@@ -685,10 +686,13 @@ obj_elf_change_section (name, type, attr
 		   && (attr &~ ssect->attr &~ SHF_MERGE &~ SHF_STRINGS) == 0)
 	    ;
 	  else
-	    as_warn (_("setting incorrect section attributes for %s"),
-		     name);
+	    {
+	      as_warn (_("setting incorrect section attributes for %s"),
+		       name);
+	      override = TRUE;
+	    }
 	}
-      if (old_sec == NULL)
+      if (!override && old_sec == NULL)
 	attr |= ssect->attr;
     }
 

Re: [PATCH] Re: Gate DSO not building properly?

[David Mosberger]

>>>>> On Wed, 22 Oct 2003 13:45:47 +1000, Ian Wienand <ianw@gelato.unsw.edu.au> said:

  Ian> On Tue, Oct 21, 2003 at 04:10:51PM +1000, Ian Wienand wrote:

  >> arch/ia64/kernel/gate-data.S:1: Warning: setting incorrect
  >> section attributes for .data.gate

  Ian> I figured out why (sorry if it's obvious); gas matches anything
  Ian> with prefix '.data.' as a data special section and flags it as
  Ian> 'SHF_ALLOC + SHF_WRITE' as per ELF [*].

It wasn't obvious to me.  I really disklike all this section-name
based matching in BFD/GAS, but it appears to ingrained to have an easy
fix.

  Ian> Issuing

  Ian> .section .data.gate, "ax"

  Ian> thus conflicts as it's assumed to be an extension of a .data
  Ian> section.

  Ian> Should this even be in data at all, considering it's code?
  Ian> What about something along the lines of the attached patch
  Ian> which puts it in it's own section?

Actually, as far as the kernel is concerned, it _is_ data.  Only when
it's mapped at the gate address is it used as text.  I'm not sure why
I put "ax" there---it was most likely a left-over from earlier
experimentation.  So I think the fix is to change "ax" to "aw" (the
data isn't really writable, but to avoid losing more memory to
page-alignment, it's better to keep the gate page in the writeable
data section).  Do you want to try this?

	--david

Re: [PATCH] Re: Gate DSO not building properly?

[Ian Wienand]

[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]

On Wed, Oct 22, 2003 at 11:08:20AM -0700, David Mosberger wrote:
>   Ian> Issuing
>   Ian> .section .data.gate, "ax"
>   Ian> thus conflicts as it's assumed to be an extension of a .data
>   Ian> section.

> I'm not sure why I put "ax" there---it was most likely a left-over
> from earlier experimentation.  So I think the fix is to change "ax"
> to "aw" (the data isn't really writable, but to avoid losing more
> memory to page-alignment, it's better to keep the gate page in the
> writeable data section).  Do you want to try this?

Yep, this stops the warning and everything looks fine.  I'd say you
put it there because it gets lumped with

        .section __special_page_section,"ax"

from head.S, so in the end the whole thing ends up with AWX.

  [21] .data.page_aligne PROGBITS         a000000100610000  00620000
       0000000000008d50  0000000000000000 WAX       0     0     1

You're of course right about wasting memory with a separate section, I
didn't think of that. 

Maybe just change it and add something like 

Even though this is put with __special_page_section which is given
attributes "ax" (head.S), gas will match anything with a prefix
'.data.' as a data special section.  Thus to stop unnecessary warning
about changing attributes, we use the .data section standard
attributes "aw" when creating gate-data.o

to the changelog?

-i
ianw@gelato.unsw.edu.au
http://www.gelato.unsw.edu.au

[-- Attachment #2: gate-data-aw.diff --]
[-- Type: text/plain, Size: 259 bytes --]

===== gate-data.S 1.1 vs edited =====
--- 1.1/arch/ia64/kernel/gate-data.S	Thu Jun 12 18:09:19 2003
+++ edited/gate-data.S	Thu Oct 23 11:51:03 2003
@@ -1,3 +1,3 @@
-	.section .data.gate, "ax"
+	.section .data.gate, "aw"
 
 	.incbin "arch/ia64/kernel/gate.so"

Re: [PATCH] Re: Gate DSO not building properly?

[David Mosberger]

Ian,

Thanks for tracking this down & the patch.  I applied it now.

	--david

>>>>> On Thu, 23 Oct 2003 11:52:30 +1000, Ian Wienand <ianw@gelato.unsw.edu.au> said:

  Ian> Maybe just change it and add something like 

  Ian> Even though this is put with __special_page_section which is given
  Ian> attributes "ax" (head.S), gas will match anything with a prefix
  Ian> '.data.' as a data special section.  Thus to stop unnecessary warning
  Ian> about changing attributes, we use the .data section standard
  Ian> attributes "aw" when creating gate-data.o

  Ian> to the changelog?

  Ian> -i
  Ian> ianw@gelato.unsw.edu.au
  Ian> http://www.gelato.unsw.edu.au
  Ian> === gate-data.S 1.1 vs edited ==  Ian> --- 1.1/arch/ia64/kernel/gate-data.S	Thu Jun 12 18:09:19 2003
  Ian> +++ edited/gate-data.S	Thu Oct 23 11:51:03 2003
  Ian> @@ -1,3 +1,3 @@
  Ian> -	.section .data.gate, "ax"
  Ian> +	.section .data.gate, "aw"
 
  Ian> .incbin "arch/ia64/kernel/gate.so"

Re: [PATCH] Re: Gate DSO not building properly?

[Nick Clifton]

Hi H.J.

>> I figured out why (sorry if it's obvious); gas matches anything with
>> prefix '.data.' as a data special section and flags it as 'SHF_ALLOC +
>> SHF_WRITE' as per ELF [*].  Issuing 
>> 
>> .section .data.gate, "ax"
>> 
>> thus conflicts as it's assumed to be an extension of a .data section.

> 2003-10-21  H.J. Lu  <hongjiu.lu@intel.com>
>
> 	* config/obj-elf.c (obj_elf_change_section): Use specified
> 	section attributes.

Approved - with a few minor changes, see below - please apply.

Cheers
        Nick

        
> +  bfd_boolean override = FALSE;

[optional]: This variable could be moved inside the if statement
starting at line 672, localizing it, if the statement was changed to:

  if (old_sec = NULL)
    {
      bfd_boolean override = FALSE;

      if (attr & ~ ssect->attr)
        {
        ...
        }

      if (!override)
	attr |= ssect->attr;
  }      

I feel that the code looks neater this way.

     
>  		   && (attr &~ ssect->attr &~ SHF_MERGE &~ SHF_STRINGS) = 0)

I would appreciate it if you could add a space between the '&' and the
'~'.  These are separate operators and should not be juxtaposed like
that.  I know that this is not part of your original patch, but since
we are modifying this area of code, I think that it would be a good
idea to clean this up.


> +	      as_warn (_("setting incorrect section attributes for %s"),
> +		       name);
> +	      override = TRUE;

Since the new attributes are going to be ignored, we should change the
warning message appropriately.  Something like:

	      as_warn (_("ignoring setting of incorrect section attributes for %s"),

Re: [PATCH] Re: Gate DSO not building properly?

[Matthew Wilcox]

On Fri, Oct 24, 2003 at 11:07:22AM +0100, Nick Clifton wrote:
> >  		   && (attr &~ ssect->attr &~ SHF_MERGE &~ SHF_STRINGS) = 0)
> 
> I would appreciate it if you could add a space between the '&' and the
> '~'.  These are separate operators and should not be juxtaposed like
> that.  I know that this is not part of your original patch, but since
> we are modifying this area of code, I think that it would be a good
> idea to clean this up.

There's two schools of thought on this one ... it really depends how
you prefer to think about it.  Myself, I'm used to ARM with its BIC
(BIt Clear) instruction, which I translate to the &~ "operator".  I
know some people prefer to think of it as "invert all the bits and mask"
but that's just less intuitive for me.

I'd have more sympathy for the latter view if it could lead to subtle
bugs, but I've been unable to come up with a situation where it makes
a difference.  The type promotion rules seem to prevent inadvertent bits
being cleared.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] Re: Gate DSO not building properly?

[Andreas Schwab]

Matthew Wilcox <willy@debian.org> writes:

> I'd have more sympathy for the latter view if it could lead to subtle
> bugs, but I've been unable to come up with a situation where it makes
> a difference.

a &~ x-1

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Re: Gate DSO not building properly?

[H. J. Lu]

On Fri, Oct 24, 2003 at 11:07:22AM +0100, Nick Clifton wrote:
> 
>         
> > +  bfd_boolean override = FALSE;
> 
> [optional]: This variable could be moved inside the if statement
> starting at line 672, localizing it, if the statement was changed to:
> 
>   if (old_sec = NULL)
>     {
>       bfd_boolean override = FALSE;
> 
>       if (attr & ~ ssect->attr)
>         {
>         ...
>         }
> 
>       if (!override)
> 	attr |= ssect->attr;
>   }      
> 
> I feel that the code looks neater this way.
> 
>      
> >  		   && (attr &~ ssect->attr &~ SHF_MERGE &~ SHF_STRINGS) = 0)
> 
> I would appreciate it if you could add a space between the '&' and the
> '~'.  These are separate operators and should not be juxtaposed like
> that.  I know that this is not part of your original patch, but since
> we are modifying this area of code, I think that it would be a good
> idea to clean this up.
> 
> 
> > +	      as_warn (_("setting incorrect section attributes for %s"),
> > +		       name);
> > +	      override = TRUE;
> 
> Since the new attributes are going to be ignored, we should change the
> warning message appropriately.  Something like:
> 
> 	      as_warn (_("ignoring setting of incorrect section attributes for %s"),

The attributes got ignored are the default ones. We are setting user-
specified, "incorrect" attributes according to special_sections in
bfd/elf.c.

I will check in the patch enclosed here.


H.J.
----
2003-10-24  H.J. Lu  <hongjiu.lu@intel.com>

	* config/obj-elf.c (obj_elf_change_section): Allow SHF_ALLOC
	for .interp, .strtab and .symtab. Use specified section
	attributes.

--- gas/config/obj-elf.c.attr	2003-09-23 08:16:44.000000000 -0700
+++ gas/config/obj-elf.c	2003-10-24 08:08:29.000000000 -0700
@@ -641,6 +641,8 @@ obj_elf_change_section (name, type, attr
 
   if (ssect != NULL)
     {
+      bfd_boolean override = FALSE;
+
       if (type = SHT_NULL)
 	type = ssect->type;
       else if (type != ssect->type)
@@ -669,7 +671,7 @@ obj_elf_change_section (name, type, attr
 	    }
 	}
 
-      if (old_sec = NULL && (attr &~ ssect->attr) != 0)
+      if (old_sec = NULL && (attr & ~ssect->attr) != 0)
 	{
 	  /* As a GNU extension, we permit a .note section to be
 	     allocatable.  If the linker sees an allocatable .note
@@ -682,13 +684,25 @@ obj_elf_change_section (name, type, attr
 	     something like .rodata.str.  */
 	  else if (ssect->suffix_length = -2
 		   && name[ssect->prefix_length] = '.'
-		   && (attr &~ ssect->attr &~ SHF_MERGE &~ SHF_STRINGS) = 0)
+		   && (attr
+		       & ~ssect->attr
+		       & ~SHF_MERGE
+		       & ~SHF_STRINGS) = 0)
 	    ;
+	  /* .interp, .strtab and .symtab can have SHF_ALLOC.  */
+	  else if (attr = SHF_ALLOC
+		   && (strcmp (name, ".interp") = 0
+		       || strcmp (name, ".strtab") = 0
+		       || strcmp (name, ".symtab") = 0))
+	    override = TRUE;
 	  else
-	    as_warn (_("setting incorrect section attributes for %s"),
-		     name);
+	    {
+	      as_warn (_("setting incorrect section attributes for %s"),
+		       name);
+	      override = TRUE;
+	    }
 	}
-      if (old_sec = NULL)
+      if (!override && old_sec = NULL)
 	attr |= ssect->attr;
     }
 

Re: [PATCH] Re: Gate DSO not building properly?

[Matthew Wilcox]

On Fri, Oct 24, 2003 at 02:21:04PM +0200, Andreas Schwab wrote:
> Matthew Wilcox <willy@debian.org> writes:
> > I'd have more sympathy for the latter view if it could lead to subtle
> > bugs, but I've been unable to come up with a situation where it makes
> > a difference.
> 
> a &~ x-1

tmp.c:12: warning: suggest parentheses around + or - in operand of &

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk