Re: PATCH: Don't allow ia64 unwind section to point to section in different files

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[Jan Beulich]

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

How about this change then?

Built and tested natively on ia64-unknown-linux-gnu and as cross tools
for ia64-hpux.

Jan

gas/
2005-05-18  Jan Beulich  <jbeulich@novell.com>

	* config/tc-ia64.c (dot_endp): Don't use global symbol for unwind
	relocations in unwind section.

gas/testsuite/
2005-05-18  Jan Beulich  <jbeulich@novell.com>

	* gas/ia64/reloc-uw.s: New.
	* gas/ia64/reloc-uw.d: New.
	* gas/ia64/reloc-uw-ilp32.d: New.
	* gas/ia64/ia64.exp: Run new test.

--- /home/jbeulich/src/binutils/mainline/2005-05-18/gas/config/tc-ia64.c	2005-05-09 08:31:38.000000000 +0200
+++ 2005-05-18/gas/config/tc-ia64.c	2005-05-18 15:47:16.019373692 +0200
@@ -4442,7 +4442,13 @@ dot_endp (dummy)
       e.X_op = O_pseudo_fixup;
       e.X_op_symbol = pseudo_func[FUNC_SEG_RELATIVE].u.sym;
       e.X_add_number = 0;
-      e.X_add_symbol = unwind.proc_start;
+      if (!S_IS_LOCAL (unwind.proc_start)
+	  && S_IS_DEFINED (unwind.proc_start))
+	e.X_add_symbol = symbol_temp_new (S_GET_SEGMENT (unwind.proc_start),
+					  S_GET_VALUE (unwind.proc_start),
+					  symbol_get_frag (unwind.proc_start));
+      else
+	e.X_add_symbol = unwind.proc_start;
       ia64_cons_fix_new (frag_now, where, bytes_per_address, &e);
 
       e.X_op = O_pseudo_fixup;
--- /home/jbeulich/src/binutils/mainline/2005-05-18/gas/testsuite/gas/ia64/ia64.exp	2005-05-09 08:31:39.000000000 +0200
+++ 2005-05-18/gas/testsuite/gas/ia64/ia64.exp	2005-05-18 15:47:16.020350255 +0200
@@ -65,11 +65,13 @@ if [istarget "ia64-*"] then {
 	run_dump_test "unwind-ilp32"
 	run_dump_test "alias-ilp32"
 	run_dump_test "xdata-ilp32"
+	run_dump_test "reloc-uw-ilp32"
     } else {
 	run_dump_test "secname"
 	run_dump_test "unwind"
 	run_dump_test "alias"
 	run_dump_test "xdata"
+	run_dump_test "reloc-uw"
 	run_dump_test "group-1"
 	run_dump_test "group-2"
     }
--- /home/jbeulich/src/binutils/mainline/2005-05-18/gas/testsuite/gas/ia64/reloc-uw-ilp32.d	1970-01-01 01:00:00.000000000 +0100
+++ 2005-05-18/gas/testsuite/gas/ia64/reloc-uw-ilp32.d	2005-05-18 15:06:17.000000000 +0200
@@ -0,0 +1,15 @@
+#objdump: -r
+#name: ia64 unwind relocations (ilp32)
+#as: -milp32
+#source: reloc-uw.s
+
+.*: +file format .*
+
+RELOCATION RECORDS FOR \[\.IA_64\.unwind\]:
+OFFSET[[:space:]]+TYPE[[:space:]]+VALUE[[:space:]]*
+0*00 SEGREL32[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*04 SEGREL32[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*08 SEGREL32[ML]SB[[:space:]]+\.IA_64\.unwind_info(\+0x[[:xdigit:]]*[048c])?
+0*0c SEGREL32[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*10 SEGREL32[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*14 SEGREL32[ML]SB[[:space:]]+\.IA_64\.unwind_info(\+0x[[:xdigit:]]*[048c])?
--- /home/jbeulich/src/binutils/mainline/2005-05-18/gas/testsuite/gas/ia64/reloc-uw.d	1970-01-01 01:00:00.000000000 +0100
+++ 2005-05-18/gas/testsuite/gas/ia64/reloc-uw.d	2005-05-18 15:05:55.000000000 +0200
@@ -0,0 +1,13 @@
+# objdump: -r
+# name: ia64 unwind relocations
+
+.*: +file format .*
+
+RELOCATION RECORDS FOR \[\.IA_64\.unwind\]:
+OFFSET[[:space:]]+TYPE[[:space:]]+VALUE[[:space:]]*
+0*00 SEGREL64[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*08 SEGREL64[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*10 SEGREL64[ML]SB[[:space:]]+\.IA_64\.unwind_info(\+0x[[:xdigit:]]*[08])?
+0*18 SEGREL64[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*20 SEGREL64[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*28 SEGREL64[ML]SB[[:space:]]+\.IA_64\.unwind_info(\+0x[[:xdigit:]]*[08])?
--- /home/jbeulich/src/binutils/mainline/2005-05-18/gas/testsuite/gas/ia64/reloc-uw.s	1970-01-01 01:00:00.000000000 +0100
+++ 2005-05-18/gas/testsuite/gas/ia64/reloc-uw.s	2005-05-18 14:50:58.000000000 +0200
@@ -0,0 +1,13 @@
+	.text
+
+	.macro uw, type
+	.proc	uw\type
+	.\type	uw\type
+uw\type:
+	.unwentry
+	br.ret.sptk rp
+	.endp	uw\type
+	.endm
+
+	uw global
+	uw weak



[-- Attachment #2: binutils-2.16-ia64-unwind-reloc.patch --]
[-- Type: text/plain, Size: 3211 bytes --]

--- /usr/local/src/binutils-2.16/gas/config/tc-ia64.c	2005-04-20 20:41:36.000000000 +0200
+++ 2.16/gas/config/tc-ia64.c	2005-05-18 14:45:21.339640840 +0200
@@ -4420,7 +4420,13 @@ dot_endp (dummy)
       e.X_op = O_pseudo_fixup;
       e.X_op_symbol = pseudo_func[FUNC_SEG_RELATIVE].u.sym;
       e.X_add_number = 0;
-      e.X_add_symbol = unwind.proc_start;
+      if (!S_IS_LOCAL (unwind.proc_start)
+	  && S_IS_DEFINED (unwind.proc_start))
+	e.X_add_symbol = symbol_temp_new (S_GET_SEGMENT (unwind.proc_start),
+					  S_GET_VALUE (unwind.proc_start),
+					  symbol_get_frag (unwind.proc_start));
+      else
+	e.X_add_symbol = unwind.proc_start;
       ia64_cons_fix_new (frag_now, where, bytes_per_address, &e);
 
       e.X_op = O_pseudo_fixup;
--- /usr/local/src/binutils-2.16/gas/testsuite/gas/ia64/ia64.exp	2005-03-08 09:27:02.000000000 +0100
+++ 2.16/gas/testsuite/gas/ia64/ia64.exp	2005-05-18 15:00:46.112054064 +0200
@@ -65,11 +65,13 @@ if [istarget "ia64-*"] then {
 	run_dump_test "unwind-ilp32"
 	run_dump_test "alias-ilp32"
 	run_dump_test "xdata-ilp32"
+	run_dump_test "reloc-uw-ilp32"
     } else {
 	run_dump_test "secname"
 	run_dump_test "unwind"
 	run_dump_test "alias"
 	run_dump_test "xdata"
+	run_dump_test "reloc-uw"
 	run_dump_test "group-1"
     }
 
--- /usr/local/src/binutils-2.16/gas/testsuite/gas/ia64/reloc-uw-ilp32.d	1970-01-01 01:00:00.000000000 +0100
+++ 2.16/gas/testsuite/gas/ia64/reloc-uw-ilp32.d	2005-05-18 15:06:17.290707256 +0200
@@ -0,0 +1,15 @@
+#objdump: -r
+#name: ia64 unwind relocations (ilp32)
+#as: -milp32
+#source: reloc-uw.s
+
+.*: +file format .*
+
+RELOCATION RECORDS FOR \[\.IA_64\.unwind\]:
+OFFSET[[:space:]]+TYPE[[:space:]]+VALUE[[:space:]]*
+0*00 SEGREL32[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*04 SEGREL32[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*08 SEGREL32[ML]SB[[:space:]]+\.IA_64\.unwind_info(\+0x[[:xdigit:]]*[048c])?
+0*0c SEGREL32[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*10 SEGREL32[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*14 SEGREL32[ML]SB[[:space:]]+\.IA_64\.unwind_info(\+0x[[:xdigit:]]*[048c])?
--- /usr/local/src/binutils-2.16/gas/testsuite/gas/ia64/reloc-uw.d	1970-01-01 01:00:00.000000000 +0100
+++ 2.16/gas/testsuite/gas/ia64/reloc-uw.d	2005-05-18 15:05:54.594157656 +0200
@@ -0,0 +1,13 @@
+# objdump: -r
+# name: ia64 unwind relocations
+
+.*: +file format .*
+
+RELOCATION RECORDS FOR \[\.IA_64\.unwind\]:
+OFFSET[[:space:]]+TYPE[[:space:]]+VALUE[[:space:]]*
+0*00 SEGREL64[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*08 SEGREL64[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*10 SEGREL64[ML]SB[[:space:]]+\.IA_64\.unwind_info(\+0x[[:xdigit:]]*[08])?
+0*18 SEGREL64[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*20 SEGREL64[ML]SB[[:space:]]+\.text(\+0x[[:xdigit:]]*0)?
+0*28 SEGREL64[ML]SB[[:space:]]+\.IA_64\.unwind_info(\+0x[[:xdigit:]]*[08])?
--- /usr/local/src/binutils-2.16/gas/testsuite/gas/ia64/reloc-uw.s	1970-01-01 01:00:00.000000000 +0100
+++ 2.16/gas/testsuite/gas/ia64/reloc-uw.s	2005-05-18 14:50:58.321411824 +0200
@@ -0,0 +1,13 @@
+	.text
+
+	.macro uw, type
+	.proc	uw\type
+	.\type	uw\type
+uw\type:
+	.unwentry
+	br.ret.sptk rp
+	.endp	uw\type
+	.endm
+
+	uw global
+	uw weak

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[H. J. Lu]

On Wed, May 18, 2005 at 03:57:56PM +0200, Jan Beulich wrote:
> How about this change then?
> 
> Built and tested natively on ia64-unknown-linux-gnu and as cross tools
> for ia64-hpux.
> 
> Jan
> 
> gas/
> 2005-05-18  Jan Beulich  <jbeulich@novell.com>
> 
> 	* config/tc-ia64.c (dot_endp): Don't use global symbol for unwind
> 	relocations in unwind section.
> 

I like this one. It works on Linux kernel.


H.J.

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[David Mosberger]

>>>>> On Wed, 18 May 2005 08:15:23 -0700, "H. J. Lu" <hjl@lucon.org> said:

  >> gas/ 2005-05-18 Jan Beulich <jbeulich@novell.com>

  >> * config/tc-ia64.c (dot_endp): Don't use global symbol for unwind
  >> relocations in unwind section.

  HJ> I like this one. It works on Linux kernel.

Cool.

BTW: I still don't quite understand what's makes this part unique to
the Linux kernel.  Wouldn't this problem potentially occur whenever
linking objects which define a symbol both in weak and strong form?

	--david

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[H. J. Lu]

On Wed, May 18, 2005 at 08:20:44AM -0700, David Mosberger wrote:
> >>>>> On Wed, 18 May 2005 08:15:23 -0700, "H. J. Lu" <hjl@lucon.org> said:
> 
>   >> gas/ 2005-05-18 Jan Beulich <jbeulich@novell.com>
> 
>   >> * config/tc-ia64.c (dot_endp): Don't use global symbol for unwind
>   >> relocations in unwind section.
> 
>   HJ> I like this one. It works on Linux kernel.
> 
> Cool.
> 
> BTW: I still don't quite understand what's makes this part unique to
> the Linux kernel.  Wouldn't this problem potentially occur whenever
> linking objects which define a symbol both in weak and strong form?

It is not unique to kernel. We just didn't notice it before.


H.J.

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[David Mosberger]

>>>>> On Wed, 18 May 2005 08:25:49 -0700, "H. J. Lu" <hjl@lucon.org> said:

  >> BTW: I still don't quite understand what's makes this part unique
  >> to the Linux kernel.  Wouldn't this problem potentially occur
  >> whenever linking objects which define a symbol both in weak and
  >> strong form?

  HJ> It is not unique to kernel. We just didn't notice it before.

OK, thanks for the clarification.  I'm really happy we have Harish's
unwind-checker script integrated into the normal Linux-kernel
build-process.  Without that, it would have taken much longer to find
the culprit.

Thanks for all the help, HJ, Jim, and Jan!

	--david

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[James E Wilson]

On Wed, 2005-05-18 at 06:57, Jan Beulich wrote:
> 	* config/tc-ia64.c (dot_endp): Don't use global symbol for unwind
> 	relocations in unwind section.

OK.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[Daniel Jacobowitz]

On Thu, May 19, 2005 at 08:29:44AM +0200, Jan Beulich wrote:
> Since this fixes a problem with building the Linux kernel, should this also go into the 2.16 branch? Thanks, Jan
> 
> >>> James E Wilson <wilson@specifixinc.com> 18.05.05 21:14:55 >>>
> On Wed, 2005-05-18 at 06:57, Jan Beulich wrote:
> > 	* config/tc-ia64.c (dot_endp): Don't use global symbol for unwind
> > 	relocations in unwind section.
> 
> OK.

The problem is already in 2.16?  Then yes, please, commit this to the
branch.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[Jan Beulich]

Since this fixes a problem with building the Linux kernel, should this also go into the 2.16 branch? Thanks, Jan

>>> James E Wilson <wilson@specifixinc.com> 18.05.05 21:14:55 >>>
On Wed, 2005-05-18 at 06:57, Jan Beulich wrote:
> 	* config/tc-ia64.c (dot_endp): Don't use global symbol for unwind
> 	relocations in unwind section.

OK.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com 

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[H. J. Lu]

On Wed, May 18, 2005 at 08:44:02AM +0200, Jan Beulich wrote:
> >I am checking with icc/ias people to see if we can find a common
> >approach.
> 
> I hope you have better luck than me; I tried to get their attention to various of the things that weren't really clear, and never heard anything back...
> 

Please let me know if you find any problems like that with icc/ias.

BTW, I understand gas and ias use different slots for brl relocation.
I don't think it is an issue. We can deal with it later if brl is ever
allowed in slot 0 in which case we will specify that the relocation
must be against slot 0.


H.J.

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[David Mosberger]

>>>>> On Wed, 18 May 2005 09:02:44 +0200, "Jan Beulich" <JBeulich@novell.com> said:

  Jan> Further (regarding the original issue discussed here), they
  Jan> generate both relocations against the symbol, not the
  Jan> section. However, I too believe this is wrong, but I'm not sure
  Jan> if gas should intentionally behave differently here. Maybe
  Jan> that's another point to be discussed with them before settling
  Jan> on a solution.

I'm not sure either, but if ias compatibility implies a risk of
silently generating wrong code, then I don't care for that
compatibility one bit.  Generating correct code is _way_ more
important.

It's good to coordinate with the IAS folks, but for now, please
restore correct behavior of GAS, since that's infinitely more
important.

Thanks,

	--david

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[Jan Beulich]

>>> James E Wilson <wilson@specifixinc.com> 18.05.05 01:20:47 >>>
>The basic question here is whether it is ever OK for anything to come
>between the .proc and the function label.

Per ias, it is OK for code and/or data to come in there, but no unwind directives. For code like

.text
.proc test1, test2, test3
.prologue
.save ar.lc, r16
	mov		r16 = ar.lc
test2:
.save pr, r17
	mov		r17 = pr
test1:	
.save.b 0b00000010, r18
	mov		r18 = b1
test3:
	br.ret.sptk	rp
.endp test1, test2, test3

ias gives

unwind.s(14) : error A2145: the first unwind directive must point to the procedure test1 entry point

basically meaning that they check that no unwind stuff exists between .proc and the main entry point (and the region size covered
by the unwind info would then be set to just the range from the main entry point through .endp).

Further (regarding the original issue discussed here), they generate both relocations against the symbol, not the section. However, I too believe this is wrong, but I'm not sure if gas should intentionally behave differently here. Maybe that's another point to be discussed with them before settling on a solution.

Jan

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[Jan Beulich]

>I am checking with icc/ias people to see if we can find a common
>approach.

I hope you have better luck than me; I tried to get their attention to various of the things that weren't really clear, and never heard anything back...

Jan

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[H. J. Lu]

On Tue, May 17, 2005 at 04:20:47PM -0700, James E Wilson wrote:
> On Tue, 2005-05-17 at 15:13, H. J. Lu wrote:
> > 	* config/tc-ia64.c (unwind): Add proc_start_addr.
> > 	(dot_proc): Set unwind.proc_start_addr to expr_build_dot ().
> > 	(dot_endp): Use unwind.proc_start_addr instead of
> > 	unwind.proc_start for unwind info.
> 
> This is an easy obvious way to get the dot value back, but this is
> undoing a change that Jan had deliberately made.  So we shouldn't be
> doing this without talking to Jan and/or re-reading the original thread
> to make sure this is OK.  Otherwise, we are just trading one problem for
> another.  I see no attempt to do any of this.
> 
> The original thread starts here:
>     http://sourceware.org/ml/binutils/2005-01/msg00380.html
> and there is a follow up thread here:
>     http://sourceware.org/ml/binutils/2005-01/msg00476.html
> This issue of using dot vs the function symbol name was discussed,
> because I pointed it out and said I wasn't sure it was safe, and Jan
> answered that it was necessary for ias compatibility.
> 
> The basic question here is whether it is ever OK for anything to come
> between the .proc and the function label.  Personally, I think that
> IA-64 assembly is complicated enough that we should only ever accept
> strictly formatted assembly code, in which case no data allocation or
> stray instructions are allowed after the .proc and before the function
> label.  Apparently, ias supports other stuff here.  If we do take a
> strict approach, then we should perhaps be emitting warnings or errors
> when we see code that doesn't meet our strict requirements, instead of
> silently generating object files with bad unwind info.  A strict
> approach also means things like switching sections inside a function are
> forbidden.
> 
> There is also a complication here with function with alternate entry
> points, as an alternate entry point could perhaps come before the
> function label, in which case using dot is perhaps better than using the
> function label, though Jan seems to argue the other way.
> 
> Anyways, if Jan has no objection to this change, then it is OK with me
> too.

I am checking with icc/ias people to see if we can find a common
approach.


H.J.

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[James E Wilson]

On Tue, 2005-05-17 at 15:13, H. J. Lu wrote:
> 	* config/tc-ia64.c (unwind): Add proc_start_addr.
> 	(dot_proc): Set unwind.proc_start_addr to expr_build_dot ().
> 	(dot_endp): Use unwind.proc_start_addr instead of
> 	unwind.proc_start for unwind info.

This is an easy obvious way to get the dot value back, but this is
undoing a change that Jan had deliberately made.  So we shouldn't be
doing this without talking to Jan and/or re-reading the original thread
to make sure this is OK.  Otherwise, we are just trading one problem for
another.  I see no attempt to do any of this.

The original thread starts here:
    http://sourceware.org/ml/binutils/2005-01/msg00380.html
and there is a follow up thread here:
    http://sourceware.org/ml/binutils/2005-01/msg00476.html
This issue of using dot vs the function symbol name was discussed,
because I pointed it out and said I wasn't sure it was safe, and Jan
answered that it was necessary for ias compatibility.

The basic question here is whether it is ever OK for anything to come
between the .proc and the function label.  Personally, I think that
IA-64 assembly is complicated enough that we should only ever accept
strictly formatted assembly code, in which case no data allocation or
stray instructions are allowed after the .proc and before the function
label.  Apparently, ias supports other stuff here.  If we do take a
strict approach, then we should perhaps be emitting warnings or errors
when we see code that doesn't meet our strict requirements, instead of
silently generating object files with bad unwind info.  A strict
approach also means things like switching sections inside a function are
forbidden.

There is also a complication here with function with alternate entry
points, as an alternate entry point could perhaps come before the
function label, in which case using dot is perhaps better than using the
function label, though Jan seems to argue the other way.

Anyways, if Jan has no objection to this change, then it is OK with me
too.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[H. J. Lu]

On Tue, May 17, 2005 at 01:38:57PM -0700, James E Wilson wrote:
> On Fri, 2005-05-13 at 14:46, H. J. Lu wrote:
> > When weak functions are used on ia64, part of the unwind section may
> > point to the strong definition in a different file. This will lead to
> > wrong unwind info. Basically, on ia64, we have to use comdat to get the
> > right unwind info. This patch will check it.
> 
> This doesn't look like the right fix to me.
> 
> I looked at the unwind info with old and new binutils, and noticed that
> the relocs are different.  In old binutils, we have
> RELOCATION RECORDS FOR [.IA_64.unwind]:
> OFFSET           TYPE              VALUE
> 0000000000000000 SEGREL64LSB       .text
> in new binutils we have
> RELOCATION RECORDS FOR [.IA_64.unwind]:
> OFFSET           TYPE              VALUE
> 0000000000000000 SEGREL64LSB       _start
> 
> Since _start is at offset 0 in .text, these are the same value normally,
> but they aren't if we have duplicate weak/strong functions.  Since the
> unwind info should always bind locally, I think it is wrong to have the
> unresolved _start symbol here.
> 
> This change comes from one of Jan Beulich's patches.  It used to be that
> unwind.proc_start was expr_build_dot(), now it is set to "sym", which is
> the symbol of the current function.  When that change was made, we
> didn't know why expr_build_dot was being used here.  Now it appears it
> was necessary to fix a problem with bad unwind info in the kernel.
> 
> Unfortunately, Jan's change was added to improve unwind info checking,
> so we can't easily change back without losing some new features.  But
> maybe there is a way to create the relocation differently, so that the
> symbol name gets resolved into a section offset in the assembler?  Or
> maybe we can create a fake local symbol with the same address to put in
> the relocation, to ensure it is resolved locally?  I am not sure of the
> best approach here.

How about this patch?


H.J.
----
2005-05-17  H.J. Lu  <hongjiu.lu@intel.com>

	* config/tc-ia64.c (unwind): Add proc_start_addr.
	(dot_proc): Set unwind.proc_start_addr to expr_build_dot ().
	(dot_endp): Use unwind.proc_start_addr instead of
	unwind.proc_start for unwind info.

--- gas/config/tc-ia64.c.unwind	2005-05-08 12:02:03.000000000 -0700
+++ gas/config/tc-ia64.c	2005-05-17 15:04:03.000000000 -0700
@@ -723,6 +723,9 @@ static struct
 
   /* These are used to create the unwind table entry for this function.  */
   symbolS *proc_start;
+  /* The unwind info has to be resolved locally even if the function is
+     global.  */
+  symbolS *proc_start_addr;
   symbolS *info;		/* pointer to unwind info */
   symbolS *personality_routine;
   segT saved_text_seg;
@@ -4298,8 +4301,9 @@ dot_proc (dummy)
 	break;
       ++input_line_pointer;
     }
+  unwind.proc_start_addr = expr_build_dot ();
   if (unwind.proc_start == 0)
-    unwind.proc_start = expr_build_dot ();
+    unwind.proc_start = unwind.proc_start_addr;
   demand_empty_rest_of_line ();
   ia64_do_align (16);
 
@@ -4442,7 +4446,7 @@ dot_endp (dummy)
       e.X_op = O_pseudo_fixup;
       e.X_op_symbol = pseudo_func[FUNC_SEG_RELATIVE].u.sym;
       e.X_add_number = 0;
-      e.X_add_symbol = unwind.proc_start;
+      e.X_add_symbol = unwind.proc_start_addr;
       ia64_cons_fix_new (frag_now, where, bytes_per_address, &e);
 
       e.X_op = O_pseudo_fixup;
@@ -4548,7 +4552,7 @@ dot_endp (dummy)
       ++input_line_pointer;
     }
   demand_empty_rest_of_line ();
-  unwind.proc_start = unwind.info = 0;
+  unwind.proc_start = unwind.proc_start_addr = unwind.info = 0;
 }
 
 static void

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[David Mosberger]

>>>>> On Tue, 17 May 2005 13:38:57 -0700, James E Wilson <wilson@specifixinc.com> said:

  Jim> On Fri, 2005-05-13 at 14:46, H. J. Lu wrote:
  >> When weak functions are used on ia64, part of the unwind section may
  >> point to the strong definition in a different file. This will lead to
  >> wrong unwind info. Basically, on ia64, we have to use comdat to get the
  >> right unwind info. This patch will check it.

  Jim> This doesn't look like the right fix to me.

  Jim> I looked at the unwind info with old and new binutils, and noticed that
  Jim> the relocs are different.  In old binutils, we have
  Jim> RELOCATION RECORDS FOR [.IA_64.unwind]:
  Jim> OFFSET           TYPE              VALUE
  Jim> 0000000000000000 SEGREL64LSB       .text
  Jim> in new binutils we have
  Jim> RELOCATION RECORDS FOR [.IA_64.unwind]:
  Jim> OFFSET           TYPE              VALUE
  Jim> 0000000000000000 SEGREL64LSB       _start

  Jim> Since _start is at offset 0 in .text, these are the same value normally,
  Jim> but they aren't if we have duplicate weak/strong functions.  Since the
  Jim> unwind info should always bind locally, I think it is wrong to have the
  Jim> unresolved _start symbol here.

Ah, so it wasn't by coincidence that the old(er) binutils worked right.
It's encouraging to see that this problem is eminently fixable.

	--david

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[James E Wilson]

On Fri, 2005-05-13 at 14:46, H. J. Lu wrote:
> When weak functions are used on ia64, part of the unwind section may
> point to the strong definition in a different file. This will lead to
> wrong unwind info. Basically, on ia64, we have to use comdat to get the
> right unwind info. This patch will check it.

This doesn't look like the right fix to me.

I looked at the unwind info with old and new binutils, and noticed that
the relocs are different.  In old binutils, we have
RELOCATION RECORDS FOR [.IA_64.unwind]:
OFFSET           TYPE              VALUE
0000000000000000 SEGREL64LSB       .text
in new binutils we have
RELOCATION RECORDS FOR [.IA_64.unwind]:
OFFSET           TYPE              VALUE
0000000000000000 SEGREL64LSB       _start

Since _start is at offset 0 in .text, these are the same value normally,
but they aren't if we have duplicate weak/strong functions.  Since the
unwind info should always bind locally, I think it is wrong to have the
unresolved _start symbol here.

This change comes from one of Jan Beulich's patches.  It used to be that
unwind.proc_start was expr_build_dot(), now it is set to "sym", which is
the symbol of the current function.  When that change was made, we
didn't know why expr_build_dot was being used here.  Now it appears it
was necessary to fix a problem with bad unwind info in the kernel.

Unfortunately, Jan's change was added to improve unwind info checking,
so we can't easily change back without losing some new features.  But
maybe there is a way to create the relocation differently, so that the
symbol name gets resolved into a section offset in the assembler?  Or
maybe we can create a fake local symbol with the same address to put in
the relocation, to ensure it is resolved locally?  I am not sure of the
best approach here.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[H. J. Lu]

On Mon, May 16, 2005 at 02:26:36AM -0700, David Mosberger wrote:
> >>>>> On Fri, 13 May 2005 14:58:15 -0700, "H. J. Lu" <hjl@lucon.org> said:
> 
>   HJ> Well, it isn't very useful. The problem is libc.a has many weak
>   HJ> functions and user can override them. My patch will make it
>   HJ> impossible on ia64. I guess we may have to live with the
>   HJ> imperfect unwind info when weak functions are used. The unwinder
>   HJ> may have to deal with it.
> 
> Huh?  How do you propose it deal with it?
> 
> Wrong unwind info will break exception handling.  I don't think that's
> acceptable.

Since the weak function is still in the executable, its unwind info is
also there. That is what you see in kernel.


H.J.

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[David Mosberger]

>>>>> On Fri, 13 May 2005 14:58:15 -0700, "H. J. Lu" <hjl@lucon.org> said:

  HJ> Well, it isn't very useful. The problem is libc.a has many weak
  HJ> functions and user can override them. My patch will make it
  HJ> impossible on ia64. I guess we may have to live with the
  HJ> imperfect unwind info when weak functions are used. The unwinder
  HJ> may have to deal with it.

Huh?  How do you propose it deal with it?

Wrong unwind info will break exception handling.  I don't think that's
acceptable.

	--david

Re: PATCH: Don't allow ia64 unwind section to point to section in different files

[H. J. Lu]

On Fri, May 13, 2005 at 02:46:12PM -0700, H. J. Lu wrote:
> On Fri, May 13, 2005 at 02:05:56PM -0700, David Mosberger wrote:
> > >>>>> On Fri, 13 May 2005 14:01:11 -0700, "H. J. Lu" <hjl@lucon.org> said:
> > 
> >   >> However, there still seems to be a binutils issues here: if this
> >   >> is something binutils cannot properly support, it should issue an
> >   >> error, not silently generate wrong code, no?
> > 
> >   HJ> I will see what I can do.
> > 
> 
> When weak functions are used on ia64, part of the unwind section may
> point to the strong definition in a different file. This will lead to
> wrong unwind info. Basically, on ia64, we have to use comdat to get the
> right unwind info. This patch will check it.
> 
> 

Well, it isn't very useful. The problem is libc.a has many weak
functions and user can override them. My patch will make it impossible
on ia64. I guess we may have to live with the imperfect unwind info
when weak functions are used. The unwinder may have to deal with it.


H.J.

PATCH: Don't allow ia64 unwind section to point to section in different files

[H. J. Lu]

On Fri, May 13, 2005 at 02:05:56PM -0700, David Mosberger wrote:
> >>>>> On Fri, 13 May 2005 14:01:11 -0700, "H. J. Lu" <hjl@lucon.org> said:
> 
>   >> However, there still seems to be a binutils issues here: if this
>   >> is something binutils cannot properly support, it should issue an
>   >> error, not silently generate wrong code, no?
> 
>   HJ> I will see what I can do.
> 

When weak functions are used on ia64, part of the unwind section may
point to the strong definition in a different file. This will lead to
wrong unwind info. Basically, on ia64, we have to use comdat to get the
right unwind info. This patch will check it.


H.J.
---
2005-05-13  H.J. Lu  <hongjiu.lu@intel.com>

	* elfxx-ia64.c (elfNN_ia64_relocate_section): Don't allow
	unwind section to point to section in different files.

--- bfd/elfxx-ia64.c.weak	2005-05-13 10:47:56.000000000 -0700
+++ bfd/elfxx-ia64.c	2005-05-13 14:44:52.000000000 -0700
@@ -4151,6 +4151,27 @@ elfNN_ia64_relocate_section (output_bfd,
       value += rel->r_addend;
       dynamic_symbol_p = elfNN_ia64_dynamic_symbol_p (h, info, r_type);
 
+      /* The unwind section and the corresponding text section have
+	 to come from the same file.  When the strong function overides
+	 a weak function, part of the unwind section may point to the
+	 wrong place.  */
+      if (h
+	  && (h->root.type == bfd_link_hash_defined
+	      || h->root.type == bfd_link_hash_defweak)
+	  && (elf_section_data (input_section)->this_hdr.sh_type
+	      == SHT_IA_64_UNWIND)
+	  && input_section->owner != h->root.u.def.section->owner)
+	{
+	  (*_bfd_error_handler)
+	    (_("%B: unwind section `%A' points to symbol `%s' defined in section '%A' in %B"),
+	     input_bfd, input_section, h->root.u.def.section,
+	     h->root.u.def.section->owner,
+	     h ? h->root.root.string
+	       : bfd_elf_sym_name (input_bfd, symtab_hdr, sym, sym_sec));
+	  ret_val = FALSE;
+	  continue;
+	}
+
       switch (r_type)
 	{
 	case R_IA64_NONE: