Re: s390 gas bug

Re: s390 gas bug

[Martin Schwidefsky]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

> > it doesn't create an R_390_PLT32 relocation and instead hardcodes
> > address of f1 above it. For static int f1 (void) { return 1; } this
> > would be right, but as f1 is global symbol, it should be possible to
> > override f1 at runtime
>
> Well, for certain file formats, yes.
>
> It sounds like the s390 gas port needs to define TC_FORCE_RELOCATION
> and then have it return true for BFD_RELOC_S390_PLT32 relocs, and
> probably some others as well.

Isn't that what tc_fix_adjustable is supposed to catch ?

blue skies,
   Martin

Linux/390 Design & Development, IBM Deutschland Entwicklung GmbH
Schönaicherstr. 220, D-71032 Böblingen, Telefon: 49 - (0)7031 - 16-2247
E-Mail: schwidefsky@de.ibm.com

Re: s390 gas bug

[Alan Modra]

On Tue, Oct 02, 2001 at 03:55:43PM +0200, Martin Schwidefsky wrote:
> 
> > > it doesn't create an R_390_PLT32 relocation and instead hardcodes
> > > address of f1 above it. For static int f1 (void) { return 1; } this
> > > would be right, but as f1 is global symbol, it should be possible to
> > > override f1 at runtime
> >
> > Well, for certain file formats, yes.
> >
> > It sounds like the s390 gas port needs to define TC_FORCE_RELOCATION
> > and then have it return true for BFD_RELOC_S390_PLT32 relocs, and
> > probably some others as well.
> 
> Isn't that what tc_fix_adjustable is supposed to catch ?
> 

Well, no, not for this particular case.  You need to define
TC_FORCE_RELOCATION to cover this situation because to the generic
code in write.c:fixup_segment, your "f1@PLT-.LT1_0" expression
looks like the subtraction of two syms in one segment.  The normal
action is to perform the subtraction and not emit a reloc.

Alan

s390 gas bug.

[Martin Schwidefsky]

Hi
I just checked in a small bug fix for the s390 assembler. The biarch switch
-m31 sets s390_arch_size to 31 instead of 32. This caused the relocation
for _GLOBAL_OFFSET_TABLE_ to go wrong for biarch compiles.

blue skies,
  Martin.


2002-10-18  Urlich Weigand  <uweigand@de.ibm.com>

	* config/tc-s390.c (md_parse_option): Set s390_arch_size to 32
	for option -m31.


Index: tc-s390.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-s390.c,v
retrieving revision 1.21
retrieving revision 1.22
diff -u -r1.21 -r1.22
--- tc-s390.c	5 Sep 2002 00:01:18 -0000	1.21
+++ tc-s390.c	18 Oct 2002 07:30:06 -0000	1.22
@@ -375,7 +375,7 @@
 	warn_areg_zero = TRUE;
 
       else if (arg != NULL && strcmp (arg, "31") == 0)
-	s390_arch_size = 31;
+	s390_arch_size = 32;
 
       else if (arg != NULL && strcmp (arg, "64") == 0)
 	s390_arch_size = 64;

Re: s390 gas bug

[Martin Schwidefsky]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 5828 bytes --]

Hi,
the TC_FORCE_RELOCATION problem is still haunting me. I now have a
patch that fixes the problem. But it is really, really ugly:

diff -urN src/gas/config/tc-s390.c src-s390/gas/config/tc-s390.c
--- binutils-2.11.90.0.27/gas/config/tc-s390.c     Tue Oct 30 16:32:00 2001
+++ binutils-2.11.90.0.27-s390/gas/config/tc-s390.c     Tue Oct 30 16:32:03
2001
@@ -1641,6 +1641,36 @@
   return 1;
 }

+/* Return true if we must always emit a reloc for a type and false if
+   there is some hope of resolving it a assembly time.  */
+int
+tc_s390_force_relocation (fixp)
+     struct fix *fixp;
+{
+  /* Ensure we emit a relocation for every reference to the global
+     offset table or to the procedure link table.  */
+  switch (fixp->fx_r_type)
+    {
+    case BFD_RELOC_390_GOT12:
+    case BFD_RELOC_32_GOT_PCREL:
+    case BFD_RELOC_32_GOTOFF:
+    case BFD_RELOC_390_GOTPC:
+    case BFD_RELOC_390_GOT16:
+    case BFD_RELOC_390_GOTPCDBL:
+    case BFD_RELOC_390_GOT64:
+    case BFD_RELOC_390_GOTENT:
+    case BFD_RELOC_390_PLT32:
+    case BFD_RELOC_390_PLT16DBL:
+    case BFD_RELOC_390_PLT32DBL:
+    case BFD_RELOC_390_PLT64:
+    case BFD_RELOC_VTABLE_INHERIT:
+    case BFD_RELOC_VTABLE_ENTRY:
+      return 1;
+    default:
+      return 0;
+    }
+}
+
 /* Apply a fixup to the object code.  This is called for all the
    fixups we generated by the call to fix_new_exp, above.  In the call
    above we used a reloc code which was the largest legal reloc code
@@ -1654,7 +1684,7 @@
 md_apply_fix3 (fixp, valuep, seg)
      fixS *fixp;
      valueT *valuep;
-     segT seg ATTRIBUTE_UNUSED;
+     segT seg;
 {
   char *where;
   valueT value;
@@ -1662,22 +1692,34 @@
   value = *valuep;
   where = fixp->fx_frag->fr_literal + fixp->fx_where;

-  if (fixp->fx_subsy != NULL)
+  if (fixp->fx_subsy != NULL)
     {
+      if ((fixp->fx_addsy != NULL
+       && S_GET_SEGMENT (fixp->fx_addsy) == S_GET_SEGMENT (fixp->fx_subsy)
+       && SEG_NORMAL (S_GET_SEGMENT (fixp->fx_addsy)))
+      || (S_GET_SEGMENT (fixp->fx_subsy) == absolute_section))
+    value += S_GET_VALUE (fixp->fx_subsy);
       if (!S_IS_DEFINED (fixp->fx_subsy))
     as_bad_where (fixp->fx_file, fixp->fx_line,
                _("unresolved fx_subsy symbol that must be resolved"));
       value -= S_GET_VALUE(fixp->fx_subsy);
-    }

-  if (fixp->fx_addsy != NULL)
+      if (S_GET_SEGMENT (fixp->fx_subsy) == seg && ! fixp->fx_pcrel)
+    value += MD_PCREL_FROM_SECTION (fixp, seg);
+    }
+
+  if (fixp->fx_addsy != NULL)
     {
-      /* `*valuep' may contain the value of the symbol on which the reloc
-     will be based; we have to remove it.  */
-      if (fixp->fx_addsy->sy_used_in_reloc
-      && S_GET_SEGMENT (fixp->fx_addsy) != absolute_section
-      && S_GET_SEGMENT (fixp->fx_addsy) != undefined_section
-      && ! bfd_is_com_section (S_GET_SEGMENT (fixp->fx_addsy)))
+      if ((fixp->fx_subsy != NULL
+       && S_GET_SEGMENT (fixp->fx_addsy) == S_GET_SEGMENT (fixp->fx_subsy)
+       && SEG_NORMAL (S_GET_SEGMENT(fixp->fx_addsy)))
+      || (S_GET_SEGMENT (fixp->fx_addsy) == seg
+          && fixp->fx_pcrel && TC_RELOC_RTSYM_LOC_FIXUP (fixp))
+      || (!fixp->fx_pcrel
+          && S_GET_SEGMENT (fixp->fx_addsy) == absolute_section)
+      || (S_GET_SEGMENT (fixp->fx_addsy) != undefined_section
+          && !bfd_is_com_section (S_GET_SEGMENT (fixp->fx_addsy))
+          && TC_FIX_ADJUSTABLE(fixp)))
     value -= S_GET_VALUE (fixp->fx_addsy);

       if (fixp->fx_pcrel)
diff -urN src/gas/config/tc-s390.h src-s390/gas/config/tc-s390.h
--- src/gas/config/tc-s390.h  Fri Jul 27 01:02:55 2001
+++ src-s390/gas/config/tc-s390.h  Tue Oct 30 16:31:00 2001
@@ -44,12 +44,14 @@
            && S_IS_DEFINED ((FIX)->fx_addsy)      \
            && ! S_IS_COMMON ((FIX)->fx_addsy))))

-#define TC_FORCE_RELOCATION(FIXP)       \
-  ((FIXP)->fx_r_type == BFD_RELOC_VTABLE_INHERIT  \
-   || (FIXP)->fx_r_type == BFD_RELOC_VTABLE_ENTRY)
+#define TC_FORCE_RELOCATION(FIXP) tc_s390_force_relocation(FIXP)
+extern int tc_s390_force_relocation PARAMS ((struct fix *));

 #define tc_fix_adjustable(X)  tc_s390_fix_adjustable(X)
 extern int tc_s390_fix_adjustable PARAMS ((struct fix *));
+
+#define TC_FIX_ADJUSTABLE(fixP) \
+  (! symbol_used_in_reloc_p ((fixP)->fx_addsy) && tc_fix_adjustable (fixP))

 /* The target BFD architecture.  */
 #define TARGET_ARCH bfd_arch_s390

The problem is that fixup_segment does too much for a certain type of
relocation that is quite common for 31 bit s/390:

.LT0:
.LC1:  .long function@PLT - .LT0

A relocation of this kind is forced but fixup_segment insists to do
   add_number += (S_GET_VALUE (add_symbolP) - S_GET_VALUE (sub_symbolP))
in the case add_symbolP is in the same segment as sub_symbolP.
I think the TC_FORCE_RELOCATION check is done too late in fixup_segment
(in other situations as well), but I do not dare to change fixup_segment.
It will certainly break some or all other architectures.
I tried to catch all condition where the value of an add_symbol or of a
sub_symbol has been errornously added to the value passed to md_apply_fix3.
The two terms are quite complicated and will definitly break as soon
as a small change is done to fixup_segment. Any chance that fixup_segment
will get reworked in the near future ?

There is another patch in the making. The elf backends for s390 & s390x
are quite old compared to where i386 is. I created more or less new version
of elf32-s390.c and elf64-s390.c that closely matches elf32-i386.c in style
and functions. The question regarding this patch is, should I check these
two files in, or has anyone the desire to proof-read them first?

blue skies,
   Martin

Linux/390 Design & Development, IBM Deutschland Entwicklung GmbH
Schönaicherstr. 220, D-71032 Böblingen, Telefon: 49 - (0)7031 - 16-2247
E-Mail: schwidefsky@de.ibm.com

Re: s390 gas bug

[Andreas Jaeger]

Alan Modra <amodra@bigpond.net.au> writes:

> On Thu, Oct 04, 2001 at 01:33:01PM +0200, Martin Schwidefsky wrote:
> > 
>> 2001-10-04  Martin Schwidefsky <schwidefsky@de.ibm.com>
>> 
>>      * gas/config/tc-s390.h (TC_FORCE_RELOCATION): Relace by a call to
>>      tc_s390_force_relocation.
>>      * gas/config/tc-s390.c (s390_force_relocation): Add function
>>      tc_s390_force_relocation that forces all GOT and PLT relocations.
>
> Looks reasonable to me.  Martin, you are the binutils s390 maintainer,
> aren't you?  Please add yourself to binutils/MAINTAINERS!  You can't
> just author this code. ;-)

Has Martin now CVS access?  If not we should give it to him...

Andreas
-- 
 Andreas Jaeger
  SuSE Labs aj@suse.de
   private aj@arthur.inka.de
    http://www.suse.de/~aj

Re: s390 gas bug

[Martin Schwidefsky]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

> Looks reasonable to me.  Martin, you are the binutils s390 maintainer,
> aren't you?  Please add yourself to binutils/MAINTAINERS!  You can't
> just author this code. ;-)
Yep, found guilty. Now everybody knows who to blame for it ...

2001-10-04  Martin Schwidefsky  <schwidefsky@de.ibm.com>

     * MAINTAINERS: Add Martin Schwidefsky as maintainer for s390 port.

diff -urN src/binutils/MAINTAINERS src-s390/binutils/MAINTAINERS
--- src/binutils/MAINTAINERS  Fri Sep 14 15:57:23 2001
+++ src-s390/binutils/MAINTAINERS  Thu Oct  4 15:15:27 2001
@@ -69,6 +69,7 @@
   MIPS           Eric Christopher <echristo@redhat.com>
   M88k           Ben Elliston <bje@redhat.com>
   PPC            Geoff Keating <geoffk@redhat.com>
+  s390           Martin Schwidefsky <schwidefsky@de.ibm.com>
   SH             Jörn Rennecke <amylaar@redhat.com>
   SH             Hans-Peter Nilsson <hp@bitrange.com>
   SPARC          Jakub Jelinek <jakub@redhat.com>

blue skies,
   Martin

Linux/390 Design & Development, IBM Deutschland Entwicklung GmbH
Schönaicherstr. 220, D-71032 Böblingen, Telefon: 49 - (0)7031 - 16-2247
E-Mail: schwidefsky@de.ibm.com

Re: s390 gas bug

[Alan Modra]

On Thu, Oct 04, 2001 at 01:33:01PM +0200, Martin Schwidefsky wrote:
> 
> 2001-10-04  Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
>      * gas/config/tc-s390.h (TC_FORCE_RELOCATION): Relace by a call to
>      tc_s390_force_relocation.
>      * gas/config/tc-s390.c (s390_force_relocation): Add function
>      tc_s390_force_relocation that forces all GOT and PLT relocations.

Looks reasonable to me.  Martin, you are the binutils s390 maintainer,
aren't you?  Please add yourself to binutils/MAINTAINERS!  You can't
just author this code. ;-)

Alan

Re: s390 gas bug

[Martin Schwidefsky]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

>Well, no, not for this particular case.  You need to define
>TC_FORCE_RELOCATION to cover this situation because to the generic
>code in write.c:fixup_segment, your "f1@PLT-.LT1_0" expression
>looks like the subtraction of two syms in one segment.  The normal
>action is to perform the subtraction and not emit a reloc.

Ok, here is a patch that force all GOT and PLT relocations:

ChangeLog for gas:

2001-10-04  Martin Schwidefsky <schwidefsky@de.ibm.com>

     * gas/config/tc-s390.h (TC_FORCE_RELOCATION): Relace by a call to
     tc_s390_force_relocation.
     * gas/config/tc-s390.c (s390_force_relocation): Add function
     tc_s390_force_relocation that forces all GOT and PLT relocations.

(See attached file: force-reloc.diff)

blue skies,
   Martin

Linux/390 Design & Development, IBM Deutschland Entwicklung GmbH
Schönaicherstr. 220, D-71032 Böblingen, Telefon: 49 - (0)7031 - 16-2247
E-Mail: schwidefsky@de.ibm.com

[-- Attachment #2: force-reloc.diff --]
[-- Type: text/x-diff, Size: 1876 bytes --]

diff -urN src/gas/config/tc-s390.c src-s390/gas/config/tc-s390.c
--- src/gas/config/tc-s390.c	Thu Sep 20 13:08:00 2001
+++ src-s390/gas/config/tc-s390.c	Thu Oct  4 12:54:52 2001
@@ -1641,6 +1641,35 @@
   return 1;
 }
 
+/* Return true if we must always emit a reloc for a type and false if
+   there is some hope of resolving it a assembly time.  */
+int
+tc_s390_force_relocation (fixp)
+     struct fix *fixp;
+{
+  switch (fixp->fx_r_type)
+    {
+    case BFD_RELOC_390_GOT12:
+    case BFD_RELOC_32_GOT_PCREL:
+    case BFD_RELOC_32_GOTOFF:
+    case BFD_RELOC_390_GOTPC:
+    case BFD_RELOC_390_GOT16:
+    case BFD_RELOC_390_GOTPCDBL:
+    case BFD_RELOC_390_GOT64:
+    case BFD_RELOC_390_GOTENT:
+    case BFD_RELOC_390_PLT32:
+    case BFD_RELOC_390_PLT16DBL:
+    case BFD_RELOC_390_PLT32DBL:
+    case BFD_RELOC_390_PLT64:
+    case BFD_RELOC_VTABLE_INHERIT:
+    case BFD_RELOC_VTABLE_ENTRY:
+      return 1;
+      
+    default:
+      return 0;
+    }
+}
+
 /* Apply a fixup to the object code.  This is called for all the
    fixups we generated by the call to fix_new_exp, above.  In the call
    above we used a reloc code which was the largest legal reloc code
diff -urN src/gas/config/tc-s390.h src-s390/gas/config/tc-s390.h
--- src/gas/config/tc-s390.h	Thu Jul 26 21:07:01 2001
+++ src-s390/gas/config/tc-s390.h	Thu Oct  4 12:30:47 2001
@@ -44,9 +44,8 @@
            && S_IS_DEFINED ((FIX)->fx_addsy)      \
            && ! S_IS_COMMON ((FIX)->fx_addsy))))
 
-#define TC_FORCE_RELOCATION(FIXP)       \
-  ((FIXP)->fx_r_type == BFD_RELOC_VTABLE_INHERIT	\
-   || (FIXP)->fx_r_type == BFD_RELOC_VTABLE_ENTRY)
+#define TC_FORCE_RELOCATION(FIXP) tc_s390_force_relocation(FIXP)
+extern int tc_s390_force_relocation PARAMS ((struct fix *));
 
 #define tc_fix_adjustable(X)  tc_s390_fix_adjustable(X)
 extern int tc_s390_fix_adjustable PARAMS ((struct fix *));

Re: s390 gas bug

[Nick Clifton]

Hi Jakub,

> .globl f1
>         .type    f1,@function
> ...
> .LT1_0:
> .LC0:
>         .long   f1@PLT-.LT1_0
> ...
> )
> 
> it doesn't create an R_390_PLT32 relocation and instead hardcodes
> address of f1 above it. For static int f1 (void) { return 1; } this
> would be right, but as f1 is global symbol, it should be possible to
> override f1 at runtime

Well, for certain file formats, yes.

It sounds like the s390 gas port needs to define TC_FORCE_RELOCATION
and then have it return true for BFD_RELOC_S390_PLT32 relocs, and
probably some others as well.

Cheers
        Nick

s390 gas bug

[Jakub Jelinek]

Hi!

s390 fails several tests in prelink testsuite, because it IMHO optimizes too
much something which it should not optimize. For code like:

int f1 (void)
{
  return 1;
}

int f2 (void)
{
  return f1 () + 1;
}

(the interesting assembly part is:
.globl f1
        .type    f1,@function
...
.LT1_0:
.LC0:
        .long   f1@PLT-.LT1_0
...
)

it doesn't create an R_390_PLT32 relocation and instead hardcodes address of
f1 above it. For static int f1 (void) { return 1; } this would be right, but
as f1 is global symbol, it should be possible to override f1 at runtime
(that's exactly what prelink test does and checks whether prelinking creates
the expected conflict fixup). When I split the above into 2 C source files,
one per function, prelinking passes the parts of the testsuite which don't
fail because of kernel bug.
Do you agree this is a bug?

	Jakub