1:30 p.m.
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. 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(a)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 */
2:16 p.m.
New subject: [PATCH] Use backtrace() instead of __builtin_return_address()
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. 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(a)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 */
A few questions on this one...
Do you know if this works OK on ia64, s390 and s390x?
For whatever reason, __builtin_return_address() used
to fail on those arches if the arg was anything but zero.
That's why it was restricted to the 4 arches above,
although that was a long time ago...
Compiling with warnings turned on yields:
defs.h:1804: warning: implicit declaration of function ‘backtrace’
I haven't looked at the glibc sources, but I'm presuming it's
a "void backtrace(int)".
And __builtin_return_address() is still being called in getbuf() on a
zero-size memory allocation.
Anyway, I'm mostly concerned about not breaking the other arches.
Dave
3:29 a.m.
* 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.
Bernhard
7:56 a.m.
New subject: [PATCH] Use backtrace() instead of __builtin_return_address()
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
8:53 a.m.
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).
Bernhard
9:11 a.m.
New subject: [PATCH] Use backtrace() instead of __builtin_return_address()
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
9:27 a.m.
* 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'. :)
Bernhard
10:11 a.m.
New subject: [PATCH] Use backtrace() instead of __builtin_return_address()
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
10:31 a.m.
* 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.
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
10:51 a.m.
New subject: [PATCH] Use backtrace() instead of __builtin_return_address()
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
11:05 a.m.
New subject: [PATCH] Use backtrace() instead of __builtin_return_address()
This is what I'm going with -- let me know if for whatever reason
it fails in your environment:
11:50 a.m.
New subject: [PATCH] Use backtrace() instead of __builtin_return_address()
* Dave Anderson <anderson(a)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
11:10 a.m.
* Dave Anderson <anderson(a)redhat.com> [2008-05-14 11:51]:
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... ;-)
You distribute crash without optimisations in Fedora/RHEL? Well, our
build system complains in that case. :-|
Bernhard
2:36 p.m.
New subject: [PATCH] Use backtrace() instead of __builtin_return_address()
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
6:10 a.m.
* Dave Anderson [2008-05-13 15:36]:
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?
Yes. I only referenced to that bug to state that the behaviour is
undefined, and it's not a gcc bug per definition.
However, I tried to track down the problem a bit. In a test program, I
encountered that when __builtin_frame_address(i) is zero, then the
program crashes when __builtin_frame_address(i+1) is executed.
However, changing that in crash doesn't help. It really crashes in
__builtin_return_address(1). Tracking that down is out of my knowledge.
I used the same compiler flags in crash and the test program.
Bernhard
6040
days inactive
6041
days old
devel@lists.crash-utility.osci.io
14 comments
2 participants
participants (2)
-
Bernhard Walle -
Dave Anderson