Bernhard Walle wrote:

When crash is compiled with gcc 4.3 and -O2, the __builtin_return_address()
causes crash to crash. See also [1] for a discussion about that.


It appears that [1] only describes a problem if you ask for more frames
than exist. Given the usage in crash, it's impossible to have a frame count
of less than 4. So it must be some other bug, no?

Dave


The gcc documentation [2] says

__builtin_return_address()

On some machines it may be impossible to determine the return address of
any function other than the current one; in such cases, or when the top
of the stack has been reached, this function will return 0 or a random
value. In addition, __builtin_frame_address may be used to determine if
the top of the stack has been reached.

This function should only be used with a nonzero argument for debugging
purposes.

Even the __builtin_frame_address() does not work here. Instead of checking
if the crash is built with -O2 and introducing new preprocessor checks here,
I use the backtrace() function which is available via glibc. This works here
(tested without the other patch which brought my attention to this bug).

Since crash only runs on Linux (IIRC), the glibc dependency should not be
a problem.

Signed-off-by: Bernhard Walle <bwalle@suse.de>


[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=165992
[2] http://gcc.gnu.org/onlinedocs/gcc/Return-Address.html

---
defs.h | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)

--- a/defs.h
+++ b/defs.h
@@ -1803,15 +1803,7 @@ struct alias_data { /* c
static inline void
save_return_address(ulong *retaddr)
{
- retaddr[0] = (ulong) __builtin_return_address(0);
-#if defined(X86) || defined(PPC) || defined(X86_64) || defined(PPC64)
- if (__builtin_frame_address(1))
- retaddr[1] = (ulong) __builtin_return_address(1);
- if (__builtin_frame_address(2))
- retaddr[2] = (ulong) __builtin_return_address(2);
- if (__builtin_frame_address(3))
- retaddr[3] = (ulong) __builtin_return_address(3);
-#endif
+ backtrace(retaddr, 4);
}

#endif /* !GDB_COMMON */


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Bernhard Walle wrote:

* Dave Anderson [2008-05-13 15:16]:

A few questions on this one...

Do you know if this works OK on ia64, s390 and s390x?


I made a test program (attached) and attached the result on all
architectures. In fact, s390 and s390x produced even 100 % correct
backtraces when the program was optimised with -O2. All other
architectures leave out frames in that case, but I think that's
limitation when frame pointers are omitted that is impossible to work
around.


Compiling with warnings turned on yields:

defs.h:1804: warning: implicit declaration of function ‘backtrace’


I forgot the <execinfo.h> include.


I haven't looked at the glibc sources, but I'm presuming it's
a "void backtrace(int)".


Indeed, it's int backtrace(void **buffer, int size). In my patch I
ignored the return value. The new patch (attached) corrects that
mistake. I'm not sure if it's necessary to zero out the rest since the
memory already was initialised with 0, but to be on the save side I
think it's not bad to do it.

Since sizeof(unsigned long) == sizeof(void *) on all architectures
crash supports, the "wrong" type of buffer should be ok. In fact,
__builtin_return_address() returns also an address, i.e. void * and
not a long. Conforming strictly to C99, we should use uintptr_t. I
don't know what the crash programming guide lines say, i.e. how old the
compiler can be where crash is used. I think we should not introduce
a C99 requirement because of that small patch here.


A few words on the test program: I used backtrace_symbols_fd() to
print out the backtrace here because I was too lazy to "port" the crash
nm magic to the test program. However, the program needs to be compiled
with -rdynamic to make that work. That's also documented in
backtrace(3) manual page. So for crash, the nm magic is a big "ugly"
but it works better than the backtrace_symbols_fd() here. But for the
test program I was only interesting in the values backtrace() produces,
not in resolving, so I think that should be ok.




Thanks for digging into this. I agree with you on all counts.

One final question: does the remaining call to __builtin_return_address(0)
in tools.c:getbuf() fail in your configuration as well?

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Bernhard Walle wrote:

Hi,

* Dave Anderson [2008-05-14 08:56]:

Thanks for digging into this. I agree with you on all counts.

One final question: does the remaining call to __builtin_return_address(0)
in tools.c:getbuf() fail in your configuration as well?


Yes. __builtin_return_address(0) works in all configurations and is
also guaranteed to work with gcc. Only __builtin_return_address(n) with
n > 0 makes problems when the frame pointer is omitted (which is the
default with -O2).



I'm confused -- you say it fails in your configuration, but then say that
passing an argument of 0 (like getbuf() does) works in all configurations.

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Bernhard Walle wrote:

* Dave Anderson [2008-05-14 10:11]:

Bernhard Walle wrote:

* Dave Anderson [2008-05-14 08:56]:

Thanks for digging into this. I agree with you on all counts.

One final question: does the remaining call to __builtin_return_address(0)
in tools.c:getbuf() fail in your configuration as well?

Yes. __builtin_return_address(0) works in all configurations and is
also guaranteed to work with gcc. Only __builtin_return_address(n) with
n > 0 makes problems when the frame pointer is omitted (which is the
default with -O2).


I'm confused -- you say it fails in your configuration, but then say that
passing an argument of 0 (like getbuf() does) works in all configurations.


Sorry, the 'yes' was wrong. I meant 'no'.



OK, that's what I thought you probably meant...

Anyway, compiling with warnings shows:

defs.h: In function ‘save_return_address’:
defs.h:1809: warning: passing argument 1 of ‘backtrace’ from incompatible pointer type
defs.h:1813: warning: assignment makes integer from pointer without a cast

which are easily addressed by casting retaddr to a (void **) and using
a 0 instead of NULL.

But now the backtrace shows save_return_address(), which is pretty useless.

For example, currently it shows this:

crash> kmem -i
...
813e041: OFFSET_verify+118
80aba32: nr_blockdev_pages+881
80aae6a: dump_kmeminfo+918
80a1452: cmd_kmem+3273
...

But with the patch, it shows:

crash> kmem -i
...
813deaa: save_return_address+25
813e03a: OFFSET_verify+118
80aba46: nr_blockdev_pages+881
80aae7e: dump_kmeminfo+918

I thought that perhaps the new code prevented save_return_address()
from being inlined. But as it turns out, even the old way the
function never was inlined.

I suppose we could go with 5 instead of 4, and have dump_trace()
skip the first one, presuming that this anomoly is not architecture-
or compiler-dependent. Or maybe make it macro?

Dave


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Bernhard Walle wrote:


* Dave Anderson [2008-05-14 11:11]:

I suppose we could go with 5 instead of 4, and have dump_trace()
skip the first one, presuming that this anomoly is not architecture-
or compiler-dependent. Or maybe make it macro?


Did you compile with some optimisation? I think I remember that gcc
only inlines code with optimisation turned on.



No -- the Makefile is used as is -- you're the one modifying things... ;-)


I think it makes sense to use

`always_inline'
Generally, functions are not inlined unless optimization is
specified. For functions declared inline, this attribute inlines
the function even if no optimization level was specified.

here, i.e. __attribute__((always_inline)). I personally prefer inline
functions over macros.


Bernhard


Well, I don't have that personal preference, ;-) and after making it a macro,
it works as expected. At least with the macro, I *know* what's going to
happen...

Dave

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

* Dave Anderson <anderson@redhat.com> [2008-05-14 12:05]:
>
>
> This is what I'm going with -- let me know if for whatever reason
> it fails in your environment:

It works fine. Thanks a lot for your effort!


Bernhard
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility