Re: [PATCH] more adjustments to elf_find_function

Re: [PATCH] more adjustments to elf_find_function

[Nick Clifton]

Hi Jan,

>>1) It does not apply the same adjustment to arm_elf_find_function in 
>>elf32-arm.c.
> 
> 
> That could be easily addressed; I wonder, however, why the same
> (generic) code exists in two places. 

Because I was lazy...


> I didn't even know there's a second
> instance of it, and for such arch-specific files I'd view this as a task
> the maintainers of the arch would have to carry out (after all it must
> have been them to decide the duplicate this and perhaps a lot more
> functionality).

True - although I think that it might be nicer if we allowed some hooks 
to be created to insert into the generic find_function routine and then 
there would be no need for a target specific version.


> I think it's right (at least I intended it to behave exactly as you
> describe it). In that place I can't judge about the meaning of symbols
> between STT_LOPROC and STT_HIPROC anyway, so considering them 'normal'
> symbols seemed more obvious to me. If an arch indeed has a symbol type
> that needs to be ignored here, then a new hook would be needed.
> In any case the state change can't be at the end of the STT_FUNC case:
> STT_OBJECT and STT_TLS (as well as any future types) ones would then be
> mis-treated, and especially wrt. future extensions I used the assumption
> that those (see the relatively new STT_TLS) would be 'normal' rather
> than 'special' in the sense used here.

OK then, in which case I have no further objections to the patch, so 
please check it in.

Cheers
   Nick

Re: [PATCH] more adjustments to elf_find_function

[Alan Modra]

I think this patch is wrong.  If we are generating object files
according to the ELF spec, then any file symbols will come first.

"STT_FILE
    Conventionally, the symbol's name gives the name of the source file
    associated with the object file. A file symbol has STB_LOCAL
    binding, its section index is SHN_ABS, and it precedes the other
    STB_LOCAL symbols for the file, if it is present."

ie. any state machine tracking whether symbols are seen before/after a
file symbol is a waste of time.  If ld -r is not placing all file
symbols at the start, then that's a bug.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [PATCH] more adjustments to elf_find_function

[Alan Modra]

On Wed, Dec 01, 2004 at 09:09:37AM +0100, Jan Beulich wrote:
> they would just introduce a little more overhead.

That's one reason why I disagree with the patch.

The others are:
- you obviously cannot reliably choose the right filename for global
  symbols if there is more than one file symbol.
- for local symbols, your code relies on ld -r continuing to leave file
  symbols unsorted, which I see as a minor violation of the ELF spec.

Unnecessary, useless code doesn't help maintainability.  Granted, this
instance isn't serious;  I'm just objecting on principle.  If you
commented the code with

/* ??? Given multiple file symbols, it is impossible to reliably
   choose the right file name for global symbols.  File symbols are
   local symbols, and thus all file symbols must sort before any
   global symbols.  The ELF spec says that a file symbol must sort
   before other local symbols, but currently ld -r doesn't do this.
   So, for ld -r output, it is possible to make a better choice of
   file name for local symbols by ignoring file symbols appearing
   after a given local symbol.  */

then given that Nick has already okayed the patch, I'd withdraw my
objections.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [PATCH] more adjustments to elf_find_function

[Jan Beulich]

>>> Alan Modra <amodra@bigpond.net.au> 30.11.04 23:42:37 >>>
>I think this patch is wrong.  If we are generating object files
>according to the ELF spec, then any file symbols will come first.

We had this discussion a couple of weeks back. I needed the changed behaviour just because of the way ld -r behaves currently (and I consider it correct).

>"STT_FILE
>    Conventionally, the symbol's name gives the name of the source file
>    associated with the object file. A file symbol has STB_LOCAL
>    binding, its section index is SHN_ABS, and it precedes the other
>    STB_LOCAL symbols for the file, if it is present."

The question is what 'source file' here stands for. If it is really what the words mean, then (for the case of multiple source files) I can only interpret the wording as implying that there (a) can be multiple file symbols and (b) each of them precedes all STB_LOCAL symbols resulting from the respective source file. The only questionable thing then would be sequences of STT_FILE symbols without intervening other symbols (which gas currently produces); I would even consider these matching the standard * the file symbol getting associated with the subsequent 'normal' ones wold be the last one from the sequence.

>ie. any state machine tracking whether symbols are seen before/after a
>file symbol is a waste of time.  If ld -r is not placing all file
>symbols at the start, then that's a bug.

You wrote, when we discussed this in October, a mail ' Why "ld -r" should not be used as a replacement for "ar" ', but only from the perspective of section treatment. Whatever you may desire to add to it symbol-wise, you should consider that ld -r is, as where I was running into problems in building the Linux kernel, being in wide-spread use regardless of whether that's what it's actually intended for. If you made it move all file symbols to the start of the symbol table, then you'd loose all association between local symbols and their original source files. If I wanted to disambiguate (say in a debugger) the symbol name by prefixing it with its source file name, that wouldn't work anymore.

Anyway, I'll wait with checking this in (regardless of Nick's approval) until we come to some sort of consensus here. I would think, however, that even if there really was a need to change the behavior of ld -r, the changes suggested here wouldn't hurt (as they don't hurt for compiler-generated or otherwise single-sourced object files), they would just introduce a little more overhead.

Jan

Re: [PATCH] more adjustments to elf_find_function

[Jan Beulich]

>I am not sure that this patch is quite right.  There are two possible 
>problems with it:
>
>1) It does not apply the same adjustment to arm_elf_find_function in 
>elf32-arm.c.

That could be easily addressed; I wonder, however, why the same
(generic) code exists in two places. I didn't even know there's a second
instance of it, and for such arch-specific files I'd view this as a task
the maintainers of the arch would have to carry out (after all it must
have been them to decide the duplicate this and perhaps a lot more
functionality).

>2) I think that the transition from state==nothing_seen to 
>state==symbol_seen might be in the wrong place.  What happens if the 
>symbols encountered are in this order:
>
>   STT_LOPROC
>   STT_FILENAME
>   STT_FUNC
>
>The first symbol will change the state to symbol_seen, but will not
set 
>'func'.  The second symbol will change the state to 
>file_after_symbol_seen so that when the third symbol is encountered 
>'filename' will be set to NULL and no filename will be reported.
>
>I think that the change from state nothing_seen to state symbol_seen 
>ought to be inside the switch() statement, at the end of STT_FUNC. 
What 
>do you think ?

I think it's right (at least I intended it to behave exactly as you
describe it). In that place I can't judge about the meaning of symbols
between STT_LOPROC and STT_HIPROC anyway, so considering them 'normal'
symbols seemed more obvious to me. If an arch indeed has a symbol type
that needs to be ignored here, then a new hook would be needed.
In any case the state change can't be at the end of the STT_FUNC case:
STT_OBJECT and STT_TLS (as well as any future types) ones would then be
mis-treated, and especially wrt. future extensions I used the assumption
that those (see the relatively new STT_TLS) would be 'normal' rather
than 'special' in the sense used here.

Jan

Re: [PATCH] more adjustments to elf_find_function

[Nick Clifton]

Hi Jan,

> bfd/
> 2004-11-30 Jan Beulich  <jbeulich@novell.com>
> 
> 	* elf.c (elf_find_function): Don't use the last file symbol
> ever,
> 	seen, but the last one seen prior to the symbol being reported.
> 	Don't report a filename at all for global symbols when that
> might
> 	be ambiguous/wrong.

I am not sure that this patch is quite right.  There are two possible 
problems with it:

1) It does not apply the same adjustment to arm_elf_find_function in 
elf32-arm.c.

2) I think that the transition from state==nothing_seen to 
state==symbol_seen might be in the wrong place.  What happens if the 
symbols encountered are in this order:

   STT_LOPROC
   STT_FILENAME
   STT_FUNC

The first symbol will change the state to symbol_seen, but will not set 
'func'.  The second symbol will change the state to 
file_after_symbol_seen so that when the third symbol is encountered 
'filename' will be set to NULL and no filename will be reported.

I think that the change from state nothing_seen to state symbol_seen 
ought to be inside the switch() statement, at the end of STT_FUNC.  What 
do you think ?

Cheers
   Nick

[PATCH] more adjustments to elf_find_function

[Jan Beulich]

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

In addition to the earlier adjustment to elf_find_function making it
actually
reported file names when possible, I found that it would only report
the last
out of potentially many filenames. This was correct for compiler and
assembler
generated object files, but not for ones resulting from ld -r (as used,
for
example, in the Linux kernel build process).

Build and tested on i686-pc-linux-gnu.

bfd/
2004-11-30 Jan Beulich  <jbeulich@novell.com>

	* elf.c (elf_find_function): Don't use the last file symbol
ever,
	seen, but the last one seen prior to the symbol being reported.
	Don't report a filename at all for global symbols when that
might
	be ambiguous/wrong.

---
/home/jbeulich/src/binutils/mainline/2004-11-30.13.31/bfd/elf.c	2004-11-25
08:36:59.000000000 +0100
+++ 2004-11-30.13.31/bfd/elf.c	2004-11-30 13:54:16.377270128 +0100
@@ -6376,13 +6376,16 @@ elf_find_function (bfd *abfd ATTRIBUTE_U
 		   const char **functionname_ptr)
 {
   const char *filename;
-  asymbol *func;
+  asymbol *func, *file;
   bfd_vma low_func;
   asymbol **p;
+  enum { nothing_seen, symbol_seen, file_after_symbol_seen } state;
 
   filename = NULL;
   func = NULL;
+  file = NULL;
   low_func = 0;
+  state = nothing_seen;
 
   for (p = symbols; *p != NULL; p++)
     {
@@ -6395,8 +6398,12 @@ elf_find_function (bfd *abfd ATTRIBUTE_U
 	default:
 	  break;
 	case STT_FILE:
-	  filename = bfd_asymbol_name (&q->symbol);
-	  break;
+	  file = &q->symbol;
+	  if (state == symbol_seen)
+	    state = file_after_symbol_seen;
+	  continue;
+	case STT_SECTION:
+	  continue;
 	case STT_NOTYPE:
 	case STT_FUNC:
 	  if (bfd_get_section (&q->symbol) == section
@@ -6405,9 +6412,18 @@ elf_find_function (bfd *abfd ATTRIBUTE_U
 	    {
 	      func = (asymbol *) q;
 	      low_func = q->symbol.value;
+	      if (file == NULL)
+		filename = NULL;
+	      else if (ELF_ST_BIND (q->internal_elf_sym.st_info) !=
STB_LOCAL
+		       && state == file_after_symbol_seen)
+		filename = NULL;
+	      else
+		filename = bfd_asymbol_name (file);
 	    }
 	  break;
 	}
+      if (state == nothing_seen)
+	state = symbol_seen;
     }
 
   if (func == NULL)


[-- Attachment #2: binutils-mainline-elf-find-function-2.patch --]
[-- Type: application/octet-stream, Size: 2130 bytes --]

In addition to the earlier adjustment to elf_find_function making it actually
reported file names when possible, I found that it would only report the last
out of potentially many filenames. This was correct for compiler and assembler
generated object files, but not for ones resulting from ld -r (as used, for
example, in the Linux kernel build process).

Build and tested on i686-pc-linux-gnu.

bfd/
2004-11-30 Jan Beulich  <jbeulich@novell.com>

	* elf.c (elf_find_function): Don't use the last file symbol ever,
	seen, but the last one seen prior to the symbol being reported.
	Don't report a filename at all for global symbols when that might
	be ambiguous/wrong.

--- /home/jbeulich/src/binutils/mainline/2004-11-30.13.31/bfd/elf.c	2004-11-25 08:36:59.000000000 +0100
+++ 2004-11-30.13.31/bfd/elf.c	2004-11-30 13:54:16.377270128 +0100
@@ -6376,13 +6376,16 @@ elf_find_function (bfd *abfd ATTRIBUTE_U
 		   const char **functionname_ptr)
 {
   const char *filename;
-  asymbol *func;
+  asymbol *func, *file;
   bfd_vma low_func;
   asymbol **p;
+  enum { nothing_seen, symbol_seen, file_after_symbol_seen } state;
 
   filename = NULL;
   func = NULL;
+  file = NULL;
   low_func = 0;
+  state = nothing_seen;
 
   for (p = symbols; *p != NULL; p++)
     {
@@ -6395,8 +6398,12 @@ elf_find_function (bfd *abfd ATTRIBUTE_U
 	default:
 	  break;
 	case STT_FILE:
-	  filename = bfd_asymbol_name (&q->symbol);
-	  break;
+	  file = &q->symbol;
+	  if (state == symbol_seen)
+	    state = file_after_symbol_seen;
+	  continue;
+	case STT_SECTION:
+	  continue;
 	case STT_NOTYPE:
 	case STT_FUNC:
 	  if (bfd_get_section (&q->symbol) == section
@@ -6405,9 +6412,18 @@ elf_find_function (bfd *abfd ATTRIBUTE_U
 	    {
 	      func = (asymbol *) q;
 	      low_func = q->symbol.value;
+	      if (file == NULL)
+		filename = NULL;
+	      else if (ELF_ST_BIND (q->internal_elf_sym.st_info) != STB_LOCAL
+		       && state == file_after_symbol_seen)
+		filename = NULL;
+	      else
+		filename = bfd_asymbol_name (file);
 	    }
 	  break;
 	}
+      if (state == nothing_seen)
+	state = symbol_seen;
     }
 
   if (func == NULL)