Re: [PATCH] EfficiencySanitizer implementation.
I assume that the compiler based instrumentation,
should be more efficient than binary instrumentation.
But, I was just interested in the process of implementation
for that tool.
Sorry for the noise.
On 10/07/2018 11:03 AM, Richard Biener wrote:
> On October 6, 2018 10:17:48 PM GMT+02:00, Denis Khalikov
> wrote:
>> Hello everyone,
>> this is a patch which implements EfficiencySanitizer aka ESan
>> into GCC. The EfficiencySanitizer tool is available in the llvm.
>> So, the main idea was to port the runtime library into GCC and
>> implement GCC compiler pass on GIMPLE IR with the same semantics
>> as llvm does on llvm IR.
>> The main difference that this patch also enables ESan under 32 bit
>> ARM CPU with some changes to runtime library.
>> Link to the RFC on the llvm-dev:
>> https://lists.llvm.org/pipermail/llvm-dev/2016-April/098355.html
>>
>> I know this patch is not acceptable into GCC trunk, so, I send
>> this patch in the weekend to don't bother anyone, but may be
>> someone will be interested. Also, I'll be very appreciated for
>> any feedback.
>> GCC should be build with --enable-checking=release.
>>
>> This patch includes:
>>
>> 1. GCC pass for the CacheFragmentation tool on the GIMPLE IR.
>> Special compiler pass instruments every memory access into the struct
>> field with gimple internal call ESAN_RECORD_ACCESS and expands
>> it in sanopt pass.
>> Creates fields counter array, each cell of that array
>> counts memory accesses to the special field. Creates array of
>> the structs, where the every instance of the struct represents meta
>> info of the real struct.
>>
>> a. Source example:
>>
>> struct node {
>> int a;
>> int b;
>> int c;
>> };
>>
>> int main () {
>> struct node c;
>> for (int i = 0; i < 100; ++i) {
>> c.a = i + 1;
>> c.b = i + 1;
>> c.c = i + 1;
>> }
>> return 0;
>> }
>>
>> b. Instrumented GIMPLE:
>>:
>> _1 = i_4 + 1;
>> .ESAN_RECORD_ACCESS (0B, 0);
>> c.a = _1;
>> _2 = i_4 + 1;
>> .ESAN_RECORD_ACCESS (0B, 1);
>> c.b = _2;
>> _3 = i_4 + 1;
>> .ESAN_RECORD_ACCESS (0B, 2);
>> c.c = _3;
>> i_11 = i_4 + 1;
>>
>> c. Assembler:
>>
>> # The fields counter array.
>> # Every cell 8 bytes long and represents the amount
>> # of the field accesses.
>>
>> .weakstruct.node$1$1$1
>> .bss
>> .align 8
>> .typestruct.node$1$1$1, @object
>> .sizestruct.node$1$1$1, 24
>> struct.node$1$1$1:
>> .zero24
>>
>> # Increment the specific cell by the field index.
>> # Actually __esan_increment, could be inlined.
>>movl$struct.node$1$1$1, %eax
>>movq%rax, %rdi
>>call__esan_increment
>>movl%ebx, -32(%rbp)
>>movl-20(%rbp), %eax
>>leal1(%rax), %ebx
>>movl$struct.node$1$1$1+8, %eax
>>movq%rax, %rdi
>>call__esan_increment
>>movl%ebx, -28(%rbp)
>>movl-20(%rbp), %eax
>>leal1(%rax), %ebx
>>movl$struct.node$1$1$1+16, %eax
>>movq%rax, %rdi
>>call__esan_increment
>>
>> # The array of the structs with
>> # meta info like size of the special struct,
>> # number of the fields and pointer to the
>> # fields counter array.
>>
>> .Lesan_info0:
>> .quad.LC0
>> .long12
>> .long3
>> .quad0
>> .quad0
>> .quad0
>> .quadstruct.node$1$1$1
>> .quad0
>>
>> __esan_init is inserted to the static constructor.
>> __esan_exit is inserted to the static destructor.
>>
>> d. Output:
>>
>> ==28719== struct node
>> ==28719== size = 12, count = 300, ratio = 2
>> ==28719== # 0: count = 100
>> ==28719== # 1: count = 100
>> ==28719== # 2: count = 100
>> ==28719==EfficiencySanitizer: total struct field access count = 300
>>
>> 2. GCC pass for the WorkingSet tool.
>> Special compiler pass instruments every memory access in the program.
>> Memory accesses are simply prepended with a function call like
>> __esan_aligned_load(addr), __esan_aligned_store(addr).
>> Also, __esan_init is inserted to the static constructor and
>> __esan_exit is inserted to the static destructor.
>>
>> a. Assembler:
>>
>> movq-32(%rbp), %rax
>> movq%rax, %rdi
>> call__esan_aligned_store1
>>
>&
Re: [PATCH] Frame pointer for arm with THUMB2 mode
Richard, thanks for the answer, got it. On 09/05/2018 07:51 PM, Richard Earnshaw (lists) wrote: On 05/09/18 17:43, Denis Khalikov wrote: Thanks for the answers. I understood that, this hack makes more mess in codegen for arm, but can you please clarify what did you mean by Only a single register can be used as the frame chain. As far as I understood, GCC for arm with THUMB2 mode uses r7 register as frame pointer register by default (with -fno-omit-frame-pointer flag), GCC for arm with ARM mode uses r11, so I didn't really propose to change the frame pointer register. On entry to a function the code has to save the existing frame register. It doesn't know (can't trivially know) whether the caller is code compiled in Arm state or Thumb state. So how can it save the caller's frame register if they are not the same? Furthermore, the 'other' frame register (ie r7 in Arm state, r11 in Thumb) is available as a call-saved register, so can contain any random value. If you try to use that random value during a frame chain walk your program will most like take an access violation. It will certainly give you a garbage frame chain. R. Thanks. On 09/05/2018 06:32 PM, Wilco Dijkstra wrote: Hi Denis, Adding support for a frame chain would require an ABI change. It would have to > work across GCC, LLVM, Arm, Thumb-1 and Thumb-2 - not a trivial amount of > effort. Clang already works that way. No, that's incorrect like Richard pointed out. Only a single register can be used as the frame chain. If we change the size of the traces to 2, it could be something like this: ... At the first example we lost the full context, from where the control/data flow comes from. If 2 is not sufficient, then try 3 or 4. It may also be feasible to only enable deeper unwinding for particular libraries so you only pay an extra cost for leaks you are interested in. The stack layout like this enables only with compile time flag (-mthumb-fp and works only together with -mthumb and -fno-omit-frame-pointer). It does not affect other codegen. But any code built like that will *always* run slower even if you don't use the sanitizer. Wilco
Re: [PATCH] Frame pointer for arm with THUMB2 mode
Thanks for the answers. I understood that, this hack makes more mess in codegen for arm, but can you please clarify what did you mean by >Only a single register can be used > as the frame chain. As far as I understood, GCC for arm with THUMB2 mode uses r7 register as frame pointer register by default (with -fno-omit-frame-pointer flag), GCC for arm with ARM mode uses r11, so I didn't really propose to change the frame pointer register. Thanks. On 09/05/2018 06:32 PM, Wilco Dijkstra wrote: Hi Denis, Adding support for a frame chain would require an ABI change. It would have to > work across GCC, LLVM, Arm, Thumb-1 and Thumb-2 - not a trivial amount of > effort. Clang already works that way. No, that's incorrect like Richard pointed out. Only a single register can be used as the frame chain. If we change the size of the traces to 2, it could be something like this: ... At the first example we lost the full context, from where the control/data flow comes from. If 2 is not sufficient, then try 3 or 4. It may also be feasible to only enable deeper unwinding for particular libraries so you only pay an extra cost for leaks you are interested in. The stack layout like this enables only with compile time flag (-mthumb-fp and works only together with -mthumb and -fno-omit-frame-pointer). It does not affect other codegen. But any code built like that will *always* run slower even if you don't use the sanitizer. Wilco
Re: [PATCH] Frame pointer for arm with THUMB2 mode
Hi Wilco,
thanks for the answer.
> Adding support for a frame chain would require an ABI change. It
would have to
> work across GCC, LLVM, Arm, Thumb-1 and Thumb-2 - not a trivial amount of
> effort.
Clang already works that way.
Please look at this commit:
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMCallingConv.td?r1=269459=269458=269459
This is an example of code which clang generates
with -mthumb -fno-omit-frame-pointer -O2.
@ %bb.0:
push{r4, r5, r6, r7, lr}
add r7, sp, #12
push.w {r8, r9, r10}
sub sp, #56
mov r8, r0
movwr0, :lower16:_ZTVN6sensor14sensor_handlerE
movtr0, :upper16:_ZTVN6sensor14sensor_handlerE
mov r9, r8
addsr0, #8
str r0, [r9], #4
The only difference is clang sets frame pointer to the r7 on the stack
instead gcc sets to lr,
but it already handles by the Sanitizers.
>
> So this sounds like the first thing to do is reducing the size of the
stack traces.
> The default is 30 which is far larger than useful. Using 1 for
example should
> always be fast (since you can use __builtin_return_address(0)) and
still get
> the function that called malloc. Also if the unwinder happens to be
too slow,
> it should be optimized and caching added etc.
>
If we change the size of the traces to 2, it could be something like this:
0xb42a50a0 is located 0 bytes inside of 88-byte region
[0xb42a50a0,0xb42a50f8)
freed by thread T0 here:
#0 0xb6a35cc7 in __interceptor_free (/usr/lib/libasan.so+0x127cc7)
#1 0xb5fa64e3 in ipc::message::unref()
(/lib/libsensord-shared.so+0xe4e3)
previously allocated by thread T0 here:
#0 0xb6a36157 in malloc (/usr/lib/libasan.so+0x128157)
#1 0xb2f8852d in accel_device::get_data(unsigned int,
sensor_data_t**, int*) (/usr/lib/sensor/libsensor-hal-tm1.so+0x1052d)
Instead this:
0xb42250a0 is located 0 bytes inside of 88-byte region
[0xb42250a0,0xb42250f8)
freed by thread T0 here:
#0 0xb6989cff in __interceptor_free (/usr/lib/libasan.so+0x127cff)
#1 0xb5fa64e3 in ipc::message::unref()
(/lib/libsensord-shared.so+0xe4e3)
#2 0xb6efbf47 in sensor::sensor_handler::notify(char const*,
sensor_data_t*, int) (/usr/bin/sensord+0x1ef47)
#3 0xb6efad43 in sensor::sensor_event_handler::handle(int, unsigned
int) (/usr/bin/sensord+0x1dd43)
#4 0xb5fa3bbb (/lib/libsensord-shared.so+0x)
#5 0xb62d9a15 in g_main_context_dispatch
(/lib/libglib-2.0.so.0+0x91a15)
#6 0xb62da2d9 (/lib/libglib-2.0.so.0+0x922d9)
#7 0xb62da9a9 in g_main_loop_run (/lib/libglib-2.0.so.0+0x929a9)
#8 0xb5fa4e1b in ipc::event_loop::run(int)
(/lib/libsensord-shared.so+0xce1b)
#9 0xb6eec9a5 in main (/usr/bin/sensord+0xf9a5)
#10 0xb5cb663b in __libc_start_main (/lib/libc.so.6+0x1663b)
previously allocated by thread T0 here:
#0 0xb698a18f in malloc (/usr/lib/libasan.so+0x12818f)
#1 0xb2f8852d in accel_device::get_data(unsigned int,
sensor_data_t**, int*) (/usr/lib/sensor/libsensor-hal-tm1.so+0x1052d)
#2 0xb6ef848f in
sensor::physical_sensor_handler::get_data(sensor_data_t**, int*)
(/usr/bin/sensord+0x1b48f)
#3 0xb6efaa51 in sensor::sensor_event_handler::handle(int, unsigned
int) (/usr/bin/sensord+0x1da51)
#4 0xb5fa3bbb (/lib/libsensord-shared.so+0x)
#5 0xb62d9a15 in g_main_context_dispatch
(/lib/libglib-2.0.so.0+0x91a15)
#6 0xb62da2d9 (/lib/libglib-2.0.so.0+0x922d9)
#7 0xb62da9a9 in g_main_loop_run (/lib/libglib-2.0.so.0+0x929a9)
#8 0xb5fa4e1b in ipc::event_loop::run(int)
(/lib/libsensord-shared.so+0xce1b)
#9 0xb6eec9a5 in main (/usr/bin/sensord+0xf9a5)
#10 0xb5cb663b in __libc_start_main (/lib/libc.so.6+0x1663b)
At the first example we lost the full context, from where the
control/data flow comes from.
>
> The issue is that the frame pointer and frame chain always add a large
> overhead even when you do not use any sanitizers. This is especially bad
> for the proposed patch - you lose much of the benefit of using Thumb-2...
>
The stack layout like this enables only with compile time flag
(-mthumb-fp and works only together with -mthumb and
-fno-omit-frame-pointer). It does not affect other codegen.
Thanks.
On 09/05/2018 03:11 PM, Wilco Dijkstra wrote:
Hi Denis,
We are working on applying Address/LeakSanitizer for the full Tizen OS
distribution. It's about ~1000 packages, ASan/LSan runtime is installed
to ld.so.preload. As we know ASan/LSan has interceptors for
allocators/deallocators such as (malloc/realloc/calloc/free) and so on.
On every allocation from user space program, ASan calls
GET_STACK_TRACE_MALLOC;
which unwinds the stack frame, and by default uses frame based stack
unwinder. So, it requires to build with "-fno-omit-frame-pointer",
switching it to default unwinder really hits the performance in our case.
So this sounds like the first thing to do is reducing the size of the stack
traces.
The default is 30
Re: [PATCH] Frame pointer for arm with THUMB2 mode
Hi Wilco,
thanks for the answer.
> Adding support for a frame chain would require an ABI change. It
would have to
> work across GCC, LLVM, Arm, Thumb-1 and Thumb-2 - not a trivial amount of
> effort.
Clang already works that way.
Please look at this commit:
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMCallingConv.td?r1=269459=269458=269459
This is an example of code which clang generates
with -mthumb -fno-omit-frame-pointer -O2.
@ %bb.0:
push{r4, r5, r6, r7, lr}
add r7, sp, #12
push.w {r8, r9, r10}
sub sp, #56
mov r8, r0
movwr0, :lower16:_ZTVN6sensor14sensor_handlerE
movtr0, :upper16:_ZTVN6sensor14sensor_handlerE
mov r9, r8
addsr0, #8
str r0, [r9], #4
The only difference is clang sets frame pointer to the r7 on the stack
instead gcc sets to lr, but it already handles by the Sanitizers.
>
> So this sounds like the first thing to do is reducing the size of the
stack traces.
> The default is 30 which is far larger than useful. Using 1 for
example should
> always be fast (since you can use __builtin_return_address(0)) and
still get
> the function that called malloc. Also if the unwinder happens to be
too slow,
> it should be optimized and caching added etc.
>
If we change the size of the traces to 2, it could be something like this:
0xb42a50a0 is located 0 bytes inside of 88-byte region
[0xb42a50a0,0xb42a50f8)
freed by thread T0 here:
#0 0xb6a35cc7 in __interceptor_free (/usr/lib/libasan.so+0x127cc7)
#1 0xb5fa64e3 in ipc::message::unref()
(/lib/libsensord-shared.so+0xe4e3)
previously allocated by thread T0 here:
#0 0xb6a36157 in malloc (/usr/lib/libasan.so+0x128157)
#1 0xb2f8852d in accel_device::get_data(unsigned int,
sensor_data_t**, int*) (/usr/lib/sensor/libsensor-hal-tm1.so+0x1052d)
Instead this:
0xb42250a0 is located 0 bytes inside of 88-byte region
[0xb42250a0,0xb42250f8)
freed by thread T0 here:
#0 0xb6989cff in __interceptor_free (/usr/lib/libasan.so+0x127cff)
#1 0xb5fa64e3 in ipc::message::unref()
(/lib/libsensord-shared.so+0xe4e3)
#2 0xb6efbf47 in sensor::sensor_handler::notify(char const*,
sensor_data_t*, int) (/usr/bin/sensord+0x1ef47)
#3 0xb6efad43 in sensor::sensor_event_handler::handle(int, unsigned
int) (/usr/bin/sensord+0x1dd43)
#4 0xb5fa3bbb (/lib/libsensord-shared.so+0x)
#5 0xb62d9a15 in g_main_context_dispatch
(/lib/libglib-2.0.so.0+0x91a15)
#6 0xb62da2d9 (/lib/libglib-2.0.so.0+0x922d9)
#7 0xb62da9a9 in g_main_loop_run (/lib/libglib-2.0.so.0+0x929a9)
#8 0xb5fa4e1b in ipc::event_loop::run(int)
(/lib/libsensord-shared.so+0xce1b)
#9 0xb6eec9a5 in main (/usr/bin/sensord+0xf9a5)
#10 0xb5cb663b in __libc_start_main (/lib/libc.so.6+0x1663b)
previously allocated by thread T0 here:
#0 0xb698a18f in malloc (/usr/lib/libasan.so+0x12818f)
#1 0xb2f8852d in accel_device::get_data(unsigned int,
sensor_data_t**, int*) (/usr/lib/sensor/libsensor-hal-tm1.so+0x1052d)
#2 0xb6ef848f in
sensor::physical_sensor_handler::get_data(sensor_data_t**, int*)
(/usr/bin/sensord+0x1b48f)
#3 0xb6efaa51 in sensor::sensor_event_handler::handle(int, unsigned
int) (/usr/bin/sensord+0x1da51)
#4 0xb5fa3bbb (/lib/libsensord-shared.so+0x)
#5 0xb62d9a15 in g_main_context_dispatch
(/lib/libglib-2.0.so.0+0x91a15)
#6 0xb62da2d9 (/lib/libglib-2.0.so.0+0x922d9)
#7 0xb62da9a9 in g_main_loop_run (/lib/libglib-2.0.so.0+0x929a9)
#8 0xb5fa4e1b in ipc::event_loop::run(int)
(/lib/libsensord-shared.so+0xce1b)
#9 0xb6eec9a5 in main (/usr/bin/sensord+0xf9a5)
#10 0xb5cb663b in __libc_start_main (/lib/libc.so.6+0x1663b)
At the first example we lost the full context, from where the
control/data flow comes from.
>
> The issue is that the frame pointer and frame chain always add a large
> overhead even when you do not use any sanitizers. This is especially bad
> for the proposed patch - you lose much of the benefit of using Thumb-2...
>
The stack layout like this enables only with compile time flag
(-mthumb-fp and works only together with -mthumb and
-fno-omit-frame-pointer). It does not affect other codegen.
Thanks.
On 09/05/2018 03:11 PM, Wilco Dijkstra wrote:
Hi Denis,
We are working on applying Address/LeakSanitizer for the full Tizen OS
distribution. It's about ~1000 packages, ASan/LSan runtime is installed
to ld.so.preload. As we know ASan/LSan has interceptors for
allocators/deallocators such as (malloc/realloc/calloc/free) and so on.
On every allocation from user space program, ASan calls
GET_STACK_TRACE_MALLOC;
which unwinds the stack frame, and by default uses frame based stack
unwinder. So, it requires to build with "-fno-omit-frame-pointer",
switching it to default unwinder really hits the performance in our case.
So this sounds like the first thing to do is reducing the size of the stack
traces.
The default is 30
[PING][PATCH] Frame pointer for arm with THUMB2 mode
Hello everyone, can someone, please, take a look on this patch https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01656.html Thanks.
Re: [PATCH] Frame pointer for arm with THUMB2 mode
Hi,
thanks for the answer.
> Switching on the frame pointer typically costs 1-2% performance, so
it's a bad
> idea to use it. However changing the frame pointer like in the
proposed patch
> will have a much larger cost - both in performance and codesize.
You'd be
> lucky if it is less than 10%. This is due to placing the frame
pointer at the top
> rather than the bottom of the frame, and that is very inefficient in
Thumb-2.
>
> You would need to unwind ~100k times a second before you might see a
> performance gain. However you pay the performance cost all the time, even
> when no unwinding is required. So I don't see this as being a good idea.
>
> If unwind performance is an issue, it would make far more sense to
solve that.
> Profiling typically hits the same functions again and again.
Callgraph profiling to
> a fixed depth hits the same functions all the time. So caching is the
obvious
> solution.
We are working on applying Address/LeakSanitizer for the full Tizen OS
distribution. It's about ~1000 packages, ASan/LSan runtime is installed
to ld.so.preload. As we know ASan/LSan has interceptors for
allocators/deallocators such as (malloc/realloc/calloc/free) and so on.
On every allocation from user space program, ASan calls
GET_STACK_TRACE_MALLOC;
which unwinds the stack frame, and by default uses frame based stack
unwinder. So, it requires to build with "-fno-omit-frame-pointer",
switching it to default unwinder really hits the performance in our case.
> Doing real unwinding is also far more accurate than frame pointer based
> unwinding (the latter doesn't handle leaf functions correctly,
entry/exit in
> non-leaf functions and shrinkwrapped functions - and this breaks
callgraph
> profiling).
I agree, but in our case, all interceptors for allocators are
leaf functions, so the frame based stack unwinder works well for us.
> So my question is, is there any point in making code run
significantly slower
> all the time and in return get inaccurate unwinding?
By default we build packages with ("-marm" "-fno-omit-frame-pointer"),
because need frame based stack unwinder for every allocation, as I said
before. As we know GCC sets fp to lr on the stack with
("-fno-omit-frame-pointer" and "-marm") and I don't really know why.
But the binary size is bigger than for thumb, so, we cannot use default
thumb frame pointer and want to reduce binary size for the full
sanitized image.
In other case clang works the same way, as I offered at the patch.
It has the same issue, but it was fixed at the end of 2017
https://bugs.llvm.org/show_bug.cgi?id=18505 (The topics starts from
discussion about APCS, but it is not the main point.)
Also, unresolved issue related to this
https://github.com/google/sanitizers/issues/640
Thanks.
On 08/28/2018 03:48 AM, Wilco Dijkstra wrote:
Hi,
But we still have an issue with performance, when we are using default
unwinder, which uses unwind tables. It could be up to 10 times faster to
use frame based stack unwinder instead "default unwinder".
Switching on the frame pointer typically costs 1-2% performance, so it's a bad
idea to use it. However changing the frame pointer like in the proposed patch
will have a much larger cost - both in performance and codesize. You'd be
lucky if it is less than 10%. This is due to placing the frame pointer at the
top
rather than the bottom of the frame, and that is very inefficient in Thumb-2.
You would need to unwind ~100k times a second before you might see a
performance gain. However you pay the performance cost all the time, even
when no unwinding is required. So I don't see this as being a good idea.
If unwind performance is an issue, it would make far more sense to solve that.
Profiling typically hits the same functions again and again. Callgraph
profiling to
a fixed depth hits the same functions all the time. So caching is the obvious
solution.
Doing real unwinding is also far more accurate than frame pointer based
unwinding (the latter doesn't handle leaf functions correctly, entry/exit in
non-leaf functions and shrinkwrapped functions - and this breaks callgraph
profiling).
So my question is, is there any point in making code run significantly slower
all the time and in return get inaccurate unwinding?
Cheers,
Wilco
[PATCH] Frame pointer for arm with THUMB2 mode
Hello everyone,
I've created the patch which sets the frame pointer to the predictable
location in the stack frame for arm with THUMB2 and non-leaf functions.
Issue:
At this moment GCC emits frame layout for arm with THUMB2 mode,
"-fno-omit-frame-pointer" and AAPCS, and does not set frame pointer to
predictable location at the function frame for non-leaf functions. And
this is right thing to do regarding to answer on my
question https://gcc.gnu.org/ml/gcc-help/2018-07/msg00093.html.
But we still have an issue with performance, when we are using default
unwinder, which uses unwind tables. It could be up to 10 times faster to
use frame based stack unwinder instead "default unwinder". As we know,
the frame based stack unwinder works good for arm with ARM32 mode
("-marm" compile-time option), because fp points to the lr on the stack,
but the binary size could be about 30%-40% larger than with THUMB2 mode
("-mthumb" compile time option). So, I think it
could be good to have an ability to use frame based stack unwinder for
thumb mode too. In this case AddressSanitizer, which uses frame based
stack unwinder, by default for every allocation/deallocation, could
benefit too.
Unresolved issue related to AddressSanitizer/LeakSanitzer:
https://github.com/google/sanitizers/issues/640
Problems:
By default arm with THUMB2 mode uses r7 register as frame pointer
register and it could be hard to set frame pointer to the predictable
location in the frame for some functions and some level of optimization.
For example interceptor for malloc in libasan built with
("-mthumb -fno-omit-frame-pointer -O2"):
000aa118 <__interceptor_malloc>:
stmdb sp!, {r4, r5, r6, r7, r8, r9, r10, r11, lr}
subwsp, sp, #1076
add r7, sp, #16
...
function body
...
addwr7, r7, #1060
mov sp, r7
ldmia.w sp!, {r4, r5, r6, r7, r8, r9, r10, r11, pc}
r7 points to sp + 16 after allocating 1076 bytes on the stack and
it's impossible to find the lr at the runtime.
In other way registers should be pushed in ascending order and we cannot
change function prologue to this:
push {r4, r5, r6, r8, r9, sl, r11, r7, lr }
And seems to me, the right solution is to make multiple push and
multiple pop in the same order (clang works that way):
push {r4, r5, r6, r7, lr}
push {r8, r9, sl, r11 }
add r7, sp, #32
subw sp, sp #1076
...
function body
...
subw r7, #32
mov sp, r7
pop {r8, r9, sl, r11 }
pop {r4, r5, r6, r7, pc }
So the r7 points to lr on the stack after function prologue and we can
find previous frame pointer by
subw r7, r7, #4
ldr r7, [r7]
at the runtime.
Verification steps:
1. I've build full Tizen OS distribution which consists of about ~1000
packages with that patch "-mthumb -fno-omit-frame-pointer -mthumb-fp
-O2" and AddressSanitizer.
2. I've built the image from those packages and verified it on real
device with armv7 ISA, and it seems works. Also stack based frame
unwinder works.
3. It's hard to write correct test case, because gcc pushes {r8... r11}
registers only on some level of optimization and the source file should
have some amount of function. So, it's kinda hard to find the small test
case. I have many preprocessed files with size about ~1kk bytes.
4. The frame layout for malloc interceptor in libasan which was built
with that patch and ("-fno-omit-frame-poiniter -O2 -mthumb -mthumb-fp")
looks like this:
000bb1f4 <__interceptor_malloc>:
push{r4, r5, r6, r7, lr}
ldr r6, [pc, #340] ; (bb34c <__interceptor_malloc+0x158>)
ldr r3, [pc, #340] ; (bb350 <__interceptor_malloc+0x15c>)
add r6, pc
stmdb sp!, {r8, r9, sl, fp}
add r7, sp, #32
subwsp, sp, #1076 ; 0x434
...
function body
...
subsr7, #32
mov sp, r7
ldmia.w sp!, {r8, r9, sl, fp}
pop {r4, r5, r6, r7, pc}
5. I've faced only one build fail related to this patch - glibc package,
but this is related to option name, some build machinery does not
recognize the "-mthumb-fp" option, and I'm not sure that glibc could be
build with frame layout like this.
It would be nice to get review for this patch by some experts. I could
miss a lot of corner cases.
Thanks.
From: Denis Khalikov
Date: Thu, 23 Aug 2018 16:33:58 +0300
Subject: [PATCH] Frame pointer for arm with THUMB2 mode.
Set frame pointer to the predictable location in the stack frame
for arm with THUMB2 mode.
2018-08-23 Denis Khalikov
* config/arm/arm.c (arm_emit_multi_reg_pop_no_return): New function.
(arm_compute_initial_elimination_offset): Add support for
TARGET_THUMB_STACK_UNWIND.
(arm_expand_prologue): Emit function prologue related to
TARGET_THUMB_STACK_UNWIND.
(thumb2_expand_return): Emit function epilogue related to
TARGET_THUMB_STACK_UNWIND.
(arm_expand_epilogue): Emit function epilogue related to
TARGET_THUMB_STACK_UNWIND.
[PATCH][PR other/86198] Libacktrace and ".note.gnu.build-id" section.
Hello,
this is a patch for PR other/86198
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86198
Thanks.
From: Denis Khalikov
Date: Mon, 18 Jun 2018 20:46:39 +0300
Subject: [PATCH] PR other/86198
* elf.c (elf_add): Increase ".note.gnu.build-id" section size
checking up to 36 bytes.
---
libbacktrace/ChangeLog | 6 ++
libbacktrace/elf.c | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
index c01c7d8..2cc31bf 100644
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog
@@ -1,3 +1,9 @@
+2018-06-18 Denis Khalikov
+
+ PR other/86198
+ * elf.c (elf_add): Increase ".note.gnu.build-id" section size
+ checking up to 36 bytes.
+
2018-04-24 H.J. Lu
* configure: Regenerated.
diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c
index 0fd5e6f..f4863f0 100644
--- a/libbacktrace/elf.c
+++ b/libbacktrace/elf.c
@@ -2868,7 +2868,7 @@ elf_add (struct backtrace_state *state, const char *filename, int descriptor,
if (note->type == NT_GNU_BUILD_ID
&& note->namesz == 4
&& strncmp (note->name, "GNU", 4) == 0
- && shdr->sh_size < 12 + ((note->namesz + 3) & ~ 3) + note->descsz)
+ && shdr->sh_size <= 12 + ((note->namesz + 3) & ~ 3) + note->descsz)
{
buildid_data = >name[0] + ((note->namesz + 3) & ~ 3);
buildid_size = note->descsz;
--
1.9.1
[PATCH][PR sanitizer/86090] Add checks for lstat and readlink to sanitizer configure.
Hello, this is a patch for PR sanitizer/86090 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86090 Thanks. From: Denis Khalikov Date: Fri, 8 Jun 2018 19:53:01 +0300 Subject: [PATCH] PR sanitizer/86090 * configure.ac: Check for lstat and readlink. * configure, config.h.in: Rebuild. --- libsanitizer/ChangeLog| 6 ++ libsanitizer/config.h.in | 6 ++ libsanitizer/configure| 2 +- libsanitizer/configure.ac | 2 +- 4 files changed, 14 insertions(+), 2 deletions(-) diff --git a/libsanitizer/ChangeLog b/libsanitizer/ChangeLog index 43eb1de..4c669dd 100644 --- a/libsanitizer/ChangeLog +++ b/libsanitizer/ChangeLog @@ -1,3 +1,9 @@ +2018-06-08 Denis Khalikov + +PR sanitizer/86090 +* configure.ac: Check for lstat and readlink. +* configure, config.h.in: Rebuild. + 2018-05-31 Matthias Klose PR sanitizer/86012 diff --git a/libsanitizer/config.h.in b/libsanitizer/config.h.in index 7195840..f716c24 100644 --- a/libsanitizer/config.h.in +++ b/libsanitizer/config.h.in @@ -43,6 +43,12 @@ /* Define to 1 if you have the header file. */ #undef HAVE_MEMORY_H +/* Define to 1 if you have the `lstat' function. */ +#undef HAVE_LSTAT + +/* Define to 1 if you have the `readlink' function. */ +#undef HAVE_READLINK + /* Define to 1 if you have the header file. */ #undef HAVE_RPC_XDR_H diff --git a/libsanitizer/configure b/libsanitizer/configure index 4695bc7..5836450 100755 --- a/libsanitizer/configure +++ b/libsanitizer/configure @@ -15509,7 +15509,7 @@ fi # Check for functions needed. -for ac_func in clock_getres clock_gettime clock_settime +for ac_func in clock_getres clock_gettime clock_settime lstat readlink do : as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh` ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var" diff --git a/libsanitizer/configure.ac b/libsanitizer/configure.ac index 0d11afd..34ba09f 100644 --- a/libsanitizer/configure.ac +++ b/libsanitizer/configure.ac @@ -93,7 +93,7 @@ AM_CONDITIONAL(TSAN_SUPPORTED, [test "x$TSAN_SUPPORTED" = "xyes"]) AM_CONDITIONAL(LSAN_SUPPORTED, [test "x$LSAN_SUPPORTED" = "xyes"]) # Check for functions needed. -AC_CHECK_FUNCS(clock_getres clock_gettime clock_settime) +AC_CHECK_FUNCS(clock_getres clock_gettime clock_settime lstat readlink) # Common libraries that we need to link against for all sanitizer libs. link_sanitizer_common='-lpthread -lm' -- 1.9.1
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Thanks for answer. I understood all points which you mentioned, but can't find last one > It seems to work > out the file name a second time, even though the file name must > already be known. Can you please show me where I've missed that, if you have a time for that. Anyway, your patch works for me. Thanks. On 09/11/2017 12:11 AM, Ian Lance Taylor wrote: On Sat, Jul 29, 2017 at 1:42 PM, Denis Khalikov <d.khali...@partner.samsung.com> wrote: Hello Ian, thanks for review. I've updated the patch, can you please take a look. Apologies again for the length of time it took to reply. I've had a hard time understanding the patch. It's quite likely that I don't understand how it works, but it seems to pass the same file descriptor to process_elf_header twice. It seems to look for debug files with the buildid in places where they will not be found. It seems to work out the file name a second time, even though the file name must already be known. I eventually just wrote my own implementation. Could you try this patch and see if it works for your cases? The patch is against current mainline. Thanks. Ian
[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello, this is a ping for that patch: https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01958.html Thanks.
[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello, this is a ping for that patch: https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01958.html Thanks.
[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello, this is a ping for that patch https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00022.html Thanks.
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
On 06/29/2017 02:59 AM, Ian Lance Taylor wrote:
On Fri, Jun 16, 2017 at 8:39 AM, Denis Khalikov
<d.khali...@partner.samsung.com> wrote:
Hello everyone,
This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631
Can some one please review attached patch.
Sorry to take so long about this. It's a lot to look at.
diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
index 096ceb6..4bd97f3 100644
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog
It's best if you don't include the ChangeLog as part of the patch.
The patch never applies cleanly anyhow. Just put the ChangeLog entry
in the e-mail or as a separate attachment. Actually I guess you did
that part, just leave ChangeLog out of the patch. Thanks.
+AC_CHECK_HEADERS(limits.h)
+
+AC_CHECK_HEADERS(sys/param.h)
May as well put these in a single AC_CHECK_HEADERS. But actually it
looks like you only want these to determine PATH_MAX, and I don't
think it's worth it. Just use 4096.
+ LINK = 1,
+ REGULAR = 2
These names, and also DYN, INVALID, and EXEC, are a little too short
and too likely to conflict with some sloppy system header file. The
enums in this code all use a prefix for each element; do that here
too.
+/* Cast from void pointer to uint32_t. */
+
+static uint32_t
+getl32 (void *p)
+{
+ char *addr = (char *) p;
+ uint32_t v = 0;
+ v = *((uint32_t *) addr);
+ return v;
+}
The comment here doesn't look right; this isn't a cast. And the
argument should really be char*. But more importantly, the name
suggests that you want a little-endian read, but this won't fetch a
little-endian value on a big-endian system. Either do a proper
little-endian fetch byte by byte, as the DWARF code already does, or
clarify the function name. I don't know what you actually need.
+/* Function that produce a crc32 value for input buffer. */
Let's move the CRC32 stuff into a different file. Add a comment
explaining how the table was generated.
+ static const unsigned long crc32_table[256]
Seems like this should be uint32_t. In any case not `unsigned long`,
which is 64 bits. In general the CRC code seems to use `unsigned
long` where I would expect `uint32_t`.
+ unsigned char buffer[8 * 1024];
This code is called from signal handlers and on threads; this array is
too long to put on the stack. Instead of looping and calling read
like this, use backtrace_get_view.
+/* Get length of the path. */
+
+static int
+pathlen (const char *buffer)
This function doesn't seem to get the length of the path, it seems to
get the length of the base name.
+ if (offset >= section_size)
+return 0;
+ crc32_debug_link = getl32 (debug_link + offset);
Seems like you should compare offset + 4 to section_size to avoid
running off the end.
+ error_callback (data, "executable file is not ELF", 0);
This and other error messages no longer seem correct, as this function
is now used for files other than the executable file.
It doesn't seem like elf_get_section_by_name should call
process_elf_header, it seems like that will do unnecessary extra work.
For that matter elf_get_section_by_name shouldn't read the section
headers and names each time, we should only do that once.
+static int
+backtrace_readlink
This function should return type_of_file, and the enum should have an
error value.
+ if (buffer[0] == '/')
Use IS_ABSOLUTE_PATH.
+ debug_descriptor
+= backtrace_open_debugfile (descriptor, filename, error_callback,
+ data, state);
If you're going to call this from fileline.c, then the function needs
to be defined in pecoff.c also. But calling it in fileline.c doesn't
seem right; why doesn't backtrace_initialize call it?
Ian
Hello Ian,
thanks for review.
I've updated the patch, can you please take a look.
Thanks.
From cbecf0d858c50aa00e5445b93fdd968e0778d225 Mon Sep 17 00:00:00 2001
From: Denis Khalikov <d.khali...@partner.samsung.com>
Date: Sat, 29 Jul 2017 22:14:48 +0300
Subject: [PATCH] PR sanitizer/77631
* elf.c (enum type_of_file): New enum.
(enum type_of_elf): New enum.
(enum debug_path): New enum.
(get_uint32): New function.
(get_crc32): New function.
(base_name_len): New function.
(check_sum): New function. Verify sum.
(process_elf_header): New function. Process elf header.
(elf_get_section_by_name): New function. Get section by name.
(backtrace_readlink): New function. Get type of file from filename.
(resolve_realname): New function. Resolve real name if file is link.
(backtrace_resolve_realname): New function. Resolve real name for any
file type.
(search_for_debugfile): New function. Search for debug file in known
paths.
(open_debugfile_by_gnulink): New function. Open debug file with
gnulink.
(hex): New function. Convert to hex.
(get_build_id_name): New function. Generate build-id name.
(open_debugfile_by_build_id): New function. Open deb
[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello, this is a ping for that patch https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00022.html Thanks.
[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello, this is a ping for that patch https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00022.html Thanks.
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello Ian,
thanks for review, I've fixed issues and updated the patch.
Can you please take a look.
Thanks.
On 06/29/2017 02:59 AM, Ian Lance Taylor wrote:
On Fri, Jun 16, 2017 at 8:39 AM, Denis Khalikov
<d.khali...@partner.samsung.com> wrote:
Hello everyone,
This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631
Can some one please review attached patch.
Sorry to take so long about this. It's a lot to look at.
diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog
index 096ceb6..4bd97f3 100644
--- a/libbacktrace/ChangeLog
+++ b/libbacktrace/ChangeLog
It's best if you don't include the ChangeLog as part of the patch.
The patch never applies cleanly anyhow. Just put the ChangeLog entry
in the e-mail or as a separate attachment. Actually I guess you did
that part, just leave ChangeLog out of the patch. Thanks.
+AC_CHECK_HEADERS(limits.h)
+
+AC_CHECK_HEADERS(sys/param.h)
May as well put these in a single AC_CHECK_HEADERS. But actually it
looks like you only want these to determine PATH_MAX, and I don't
think it's worth it. Just use 4096.
+ LINK = 1,
+ REGULAR = 2
These names, and also DYN, INVALID, and EXEC, are a little too short
and too likely to conflict with some sloppy system header file. The
enums in this code all use a prefix for each element; do that here
too.
+/* Cast from void pointer to uint32_t. */
+
+static uint32_t
+getl32 (void *p)
+{
+ char *addr = (char *) p;
+ uint32_t v = 0;
+ v = *((uint32_t *) addr);
+ return v;
+}
The comment here doesn't look right; this isn't a cast. And the
argument should really be char*. But more importantly, the name
suggests that you want a little-endian read, but this won't fetch a
little-endian value on a big-endian system. Either do a proper
little-endian fetch byte by byte, as the DWARF code already does, or
clarify the function name. I don't know what you actually need.
+/* Function that produce a crc32 value for input buffer. */
Let's move the CRC32 stuff into a different file. Add a comment
explaining how the table was generated.
+ static const unsigned long crc32_table[256]
Seems like this should be uint32_t. In any case not `unsigned long`,
which is 64 bits. In general the CRC code seems to use `unsigned
long` where I would expect `uint32_t`.
+ unsigned char buffer[8 * 1024];
This code is called from signal handlers and on threads; this array is
too long to put on the stack. Instead of looping and calling read
like this, use backtrace_get_view.
+/* Get length of the path. */
+
+static int
+pathlen (const char *buffer)
This function doesn't seem to get the length of the path, it seems to
get the length of the base name.
+ if (offset >= section_size)
+return 0;
+ crc32_debug_link = getl32 (debug_link + offset);
Seems like you should compare offset + 4 to section_size to avoid
running off the end.
+ error_callback (data, "executable file is not ELF", 0);
This and other error messages no longer seem correct, as this function
is now used for files other than the executable file.
It doesn't seem like elf_get_section_by_name should call
process_elf_header, it seems like that will do unnecessary extra work.
For that matter elf_get_section_by_name shouldn't read the section
headers and names each time, we should only do that once.
+static int
+backtrace_readlink
This function should return type_of_file, and the enum should have an
error value.
+ if (buffer[0] == '/')
Use IS_ABSOLUTE_PATH.
+ debug_descriptor
+= backtrace_open_debugfile (descriptor, filename, error_callback,
+ data, state);
If you're going to call this from fileline.c, then the function needs
to be defined in pecoff.c also. But calling it in fileline.c doesn't
seem right; why doesn't backtrace_initialize call it?
Ian
From 053f4fab56ed45dec17c1d4c66868678c817acf9 Mon Sep 17 00:00:00 2001
From: Denis Khalikov <d.khali...@partner.samsung.com>
Date: Sat, 1 Jul 2017 23:37:34 +0300
Subject: [PATCH] PR sanitizer/77631
* elf.c (enum type_of_file): New enum.
(enum type_of_elf): New enum.
(enum debug_path): New enum.
(get_uint32): New function.
(get_crc32): New function.
(base_name_len): New function.
(check_sum): New function. Verify sum.
(process_elf_header): New function. Process elf header.
(elf_get_section_by_name): New function. Get section by name.
(backtrace_readlink): New function. Get type of file from filename.
(resolve_realname): New function. Resolve real name if file is link.
(backtrace_resolve_realname): New function. Resolve real name for any
file type.
(search_for_debugfile): New function. Search for debug file in known
paths.
(open_debugfile_by_gnulink): New function. Open debug file with
gnulink.
(hex): New function. Convert to hex.
(get_build_id_name): New function. Generate build-id name.
(open_debugfile_by_build_id): New function. Open deb
[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello everyone, this is a ping for patch https://gcc.gnu.org/ml/gcc-patches/2017-06/msg01209.html
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello Matthias,
thanks for review.
As far as I understood that build-id should look like this:
https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html
"For the “build ID” method, GDB looks in the .build-id subdirectory of
each one of the global debug directories for a file named
nn/.debug, where nn are the first 2 hex characters of the build
ID bit string, and are the rest of the bit string. (Real build
ID strings are 32 or more hex characters, not 10.)"
I also check gdb internals, for PR binutils/20876, which you provided.
1926 build_id = get_build_id (abfd);
1927 if (build_id == NULL)
1928 return NULL;
1929
1930 /* Compute the debug pathname corresponding to the build-id. */
1931 name = bfd_malloc (strlen (".build-id/") + build_id->size * 2 + 2
+ strlen (".debug"));
1932 if (name == NULL)
1933 {
1934 bfd_set_error (bfd_error_no_memory);
1935 return NULL;
1936 }
1937 n = name;
1938 d = build_id->data;
1939 s = build_id->size;
1940
1941 n += sprintf (n, ".build-id/");
1942 n += sprintf (n, "%02x", (unsigned) *d++); s--;
1943 n += sprintf (n, "/");
1944 while (s--)
1945 n += sprintf (n, "%02x", (unsigned) *d++);
1946 n += sprintf (n, ".debug");
1947
In my patch I do the same, in case we can't use printf functions family,
because we can't use malloc.
972 debug_postfix = ".debug";
973 debug_prefix = ".build-id/";
...
990 memset (build_id_name, 0, *len);
991 memcpy (build_id_name, debug_prefix, debug_prefix_len);
992 temp = build_id_name + debug_prefix_len;
993
994 *temp++ = hex ((*hash_start & 0xF0) >> 4);
995 *temp++ = hex (*hash_start & 0x0F);
996 ++hash_start;
997 --hash_size;
998
999 memcpy (temp, "/", 1);
1000 ++temp;
1001
1002 while (hash_size--)
1003 {
1004 *temp++ = hex ((*hash_start & 0xF0) >> 4);
1005 *temp++ = hex (*hash_start & 0x0F);
1006 ++hash_start;
1007 }
In this case if we have binary with build id:
0x0123456789abcdef0123456789abcdef01234567
For example we can use linker option to specify build-id manually:
-Wl,--build-id=0x0123456789abcdef0123456789abcdef01234567
I expect to find .build-id link to debug file at least at path
/usr/lib/debug/.build-id/01/23456789abcdef0123456789abcdef01234567.debug
May be I'am missing something ?
I also can provide 3 more tests for this patch, but it will
increase patch size in about 1,5 times.
Thanks.
On 06/18/2017 05:08 PM, Matthias Klose wrote:
On 16.06.2017 17:39, Denis Khalikov wrote:
Hello everyone,
This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631
Can some one please review attached patch.
not a full review, but it looks like the system debug files based on build-id's
are not found. In newer distro releases you find these at
$ ls /usr/lib/debug/.build-id/
00/ 0d/ 1a/ 25/ 2e/ 3a/ 45/ 4d/ 57/ 64/ 6d/ 77/ 82/ 8b/ 94/ a0/ a9/ b7/ c1/ cd/
d6/ e0/ eb/ f6/
01/ 0e/ 1b/ 26/ 2f/ 3b/ 46/ 4e/ 58/ 65/ 6e/ 78/ 83/ 8c/ 95/ a1/ ac/ b9/ c4/ ce/
d7/ e1/ ec/ f7/
02/ 11/ 1c/ 27/ 30/ 3d/ 47/ 50/ 59/ 66/ 6f/ 79/ 84/ 8e/ 96/ a2/ ae/ ba/ c5/ d0/
d8/ e3/ ee/ f9/
03/ 15/ 1d/ 28/ 32/ 3e/ 48/ 51/ 5b/ 67/ 70/ 7a/ 85/ 8f/ 99/ a3/ b0/ bb/ c6/ d1/
d9/ e4/ ef/ fb/
05/ 16/ 1e/ 29/ 35/ 41/ 49/ 52/ 5c/ 68/ 72/ 7b/ 87/ 90/ 9a/ a4/ b1/ bc/ c8/ d2/
db/ e5/ f1/ fc/
08/ 17/ 1f/ 2a/ 36/ 42/ 4a/ 53/ 5e/ 69/ 73/ 7c/ 88/ 91/ 9b/ a5/ b2/ be/ c9/ d3/
dc/ e6/ f2/ fd/
09/ 18/ 20/ 2b/ 37/ 43/ 4b/ 54/ 60/ 6a/ 75/ 7f/ 89/ 92/ 9c/ a6/ b4/ bf/ cb/ d4/
dd/ e7/ f3/ fe/
0a/ 19/ 24/ 2d/ 39/ 44/ 4c/ 55/ 61/ 6b/ 76/ 80/ 8a/ 93/ 9f/ a7/ b5/ c0/ cc/ d5/
de/ e8/ f5/ ff/
the first two bytes of the crc make the sub dir name, the debug file name has
these first two bytes omitted.
$ ls /usr/lib/debug/.build-id/0d
c55467dc9eb81a00c7715a790844e7cf035345.debug
objdump/objcopy in binutils 2.28 has these lookups properly working.
Matthias
[PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello everyone, This is a patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631 Can some one please review attached patch. Thanks. From ae74cf3d632b06a91a986e32e3a6c16223767b24 Mon Sep 17 00:00:00 2001 From: Denis Khalikov <d.khali...@partner.samsung.com> Date: Fri, 16 Jun 2017 12:13:13 +0300 Subject: [PATCH] PR sanitizer/77631 * Makefile.in: Regenerated. * configure.ac: Add searching for limits.h, sys/param.h * config.h.in: Regenerated. * configure: Regenerated. * elf.c (enum type_of_file): New enum. (enum type_of_elf): New enum. (enum debug_path): New enum. (getl32): New function. (gnu_debuglink_crc32): New function. Generate crc32 sum. (get_crc32): New function. (pathlen): New function. (check_sum): New function. Verify sum. (process_elf_header): New function. Verify elf header. (elf_get_section_by_name): New function. Get section by name. (backtrace_readlink): New function. Get type of file from filename. (resolve_realname): New function. Resolve real name if file is link. (backtrace_resolve_realname): New function. Resolve real name for any file type. (search_for_debugfile): New function. Search for debug file in known paths. (open_debugfile_by_gnulink): New function. Open debug file with gnulink. (hex): New function. Convert to hex. (get_build_id_name): New function. Generate build-id name. (open_debugfile_by_build_id): New function. Open debug file with build-id. (backtrace_open_debugfile): New function. Open debug file. (elf_add): Move code which reads elf header to elf_header_is_valid. (phdr_callback): Call backtrace_open_debugfile function for shared library. * fileline.c (fileline_initialize): Call backtrace_open_debugfile for executable. * internal.h: Updated. --- libbacktrace/ChangeLog| 37 ++ libbacktrace/Makefile.in | 2 +- libbacktrace/config.h.in | 6 + libbacktrace/configure| 26 ++ libbacktrace/configure.ac | 4 + libbacktrace/elf.c| 949 +- libbacktrace/fileline.c | 13 +- libbacktrace/internal.h | 8 + 8 files changed, 958 insertions(+), 87 deletions(-) diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog index 096ceb6..4bd97f3 100644 --- a/libbacktrace/ChangeLog +++ b/libbacktrace/ChangeLog @@ -1,3 +1,40 @@ +2017-06-16 Denis Khalikov <d.khali...@partner.samsung.com> + + PR sanitizer/77631 + * Makefile.in: Regenerated. + * configure.ac: Add searching for limits.h, sys/param.h + * config.h.in: Regenerated. + * configure: Regenerated. + * elf.c (enum type_of_file): New enum. + (enum type_of_elf): New enum. + (enum debug_path): New enum. + (getl32): New function. + (gnu_debuglink_crc32): New function. Generate crc32 sum. + (get_crc32): New function. + (pathlen): New function. + (check_sum): New function. Verify sum. + (process_elf_header): New function. Verify elf header. + (elf_get_section_by_name): New function. Get section by name. + (backtrace_readlink): New function. Get type of file from filename. + (resolve_realname): New function. Resolve real name if file is link. + (backtrace_resolve_realname): New function. Resolve real name for any + file type. + (search_for_debugfile): New function. Search for debug file in known + paths. + (open_debugfile_by_gnulink): New function. Open debug file with + gnulink. + (hex): New function. Convert to hex. + (get_build_id_name): New function. Generate build-id name. + (open_debugfile_by_build_id): New function. Open debug file with + build-id. + (backtrace_open_debugfile): New function. Open debug file. + (elf_add): Move code which reads elf header to elf_header_is_valid. + (phdr_callback): Call backtrace_open_debugfile function for shared + library. + * fileline.c (fileline_initialize): Call backtrace_open_debugfile for + executable. + * internal.h: Updated. + 2017-05-02 Release Manager * GCC 7.1.0 released. diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in index de74b5d..e604b6c 100644 --- a/libbacktrace/Makefile.in +++ b/libbacktrace/Makefile.in @@ -16,7 +16,7 @@ @SET_MAKE@ # Makefile.am -- Backtrace Makefile. -# Copyright (C) 2012-2016 Free Software Foundation, Inc. +# Copyright (C) 2012-2017 Free Software Foundation, Inc. # Redistribution and use in source and binary forms, with or without # modification, are permitted provided that the following conditions are diff --git a/libbacktrace/config.h.in b/libbacktrace/config.h.in index 87cb805..edbd9af 100644 --- a/libbacktrace/config.h.in +++ b/libbacktrace/config.h.in @@ -28,6 +28,9 @@ /* Define to 1 if you have the header file. */ #undef HAVE_INTTYPES_H +/* Define 1 if is available. */ +#undef HAVE_LIMITS_H + /* Define to 1 if you have the header file. */ #undef HAVE_LINK_H @@ -52,6 +55,9 @@ /* Define to 1 if you have the header file. */ #undef HAVE_SYS_MMAN_H +/* Define 1 if is available. */ +#undef HAVE_SYS_PARAM_H + /* Define to 1 if you have the header file. */ #undef HAVE_SYS_STAT_H
[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello everyone, this is a ping for that patch https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01462.html Can someone please review that patch. Thanks.
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello everyone, i've updated the patch recently for this issue: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77631 Can someone please review it. Thanks. On 04/13/2017 01:07 AM, Jeff Law wrote: Given this doesn't look like a regression, I'm going to punt to gcc-8. jeff From: Denis Khalikov <d.khali...@partner.samsung.com> Date: Thu, 18 May 2017 16:04:17 +0300 Subject: [PATCH] PR sanitizer/77631 * Makefile.am: Regenerated. * Makefile.in: Regenerated. * configure.ac: Add searching for limits.h and sys/param.h * config.h.in: Regenerated. * configure: Regenerated. * elf.c (enum type_of_file): New enum. * elf.c (enum_type_of_elf): New enum. (enum debug_path): New enum. (getl32): New function. (gnu_debuglink_crc32): New function. Generate crc32 sum. (get_crc32): New function. (pathlen): New function. (check_sum): New function. Verify sum. (process_elf_header): New function. Verify elf header. (elf_get_section_by_name): New function. Get section by name. (backtrace_readlink): New function. Get type of file from filename. (resolve_realname): New function. Resolve real name if file is link. (backtrace_resolve_realname): New function. Resolve real name for any file type. (search_for_debugfile): New function. Search for debug file in known paths. (open_debugfile_by_gnulink): New function. Open debug file with gnulink. (hex): New function. Convert to hex. (get_build_id_name): New function. Generate build-id name. (open_debugfile_by_build_id): New function. Open debug file with build-id. (backtrace_open_debugfile): New function. Open debug file. (elf_add): Move code which reads elf header to elf_header_is_valid. (phdr_callback): Call backtrace_open_debugfile function for shared library. * fileline.c (fileline_initialize): Call backtrace_open_debugfile for executable. * internal.h: Updated. --- libbacktrace/ChangeLog| 38 ++ libbacktrace/Makefile.in | 2 +- libbacktrace/config.h.in | 6 + libbacktrace/configure| 18 + libbacktrace/configure.ac | 6 + libbacktrace/elf.c| 977 +- libbacktrace/fileline.c | 11 +- libbacktrace/internal.h | 5 + 8 files changed, 971 insertions(+), 92 deletions(-) diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog index 25cd921..33bbf63 100644 --- a/libbacktrace/ChangeLog +++ b/libbacktrace/ChangeLog @@ -1,3 +1,41 @@ +2017-05-18 Denis Khalikov <d.khali...@partner.samsung.com> + + PR sanitizer/77631 + * Makefile.am: Regenerated. + * Makefile.in: Regenerated. + * configure.ac: Add searching for limits.h and sys/param.h + * config.h.in: Regenerated. + * configure: Regenerated. + * elf.c (enum type_of_file): New enum. + * elf.c (enum_type_of_elf): New enum. + (enum debug_path): New enum. + (getl32): New function. + (gnu_debuglink_crc32): New function. Generate crc32 sum. + (get_crc32): New function. + (pathlen): New function. + (check_sum): New function. Verify sum. + (process_elf_header): New function. Verify elf header. + (elf_get_section_by_name): New function. Get section by name. + (backtrace_readlink): New function. Get type of file from filename. + (resolve_realname): New function. Resolve real name if file is link. + (backtrace_resolve_realname): New function. Resolve real name for any + file type. + (search_for_debugfile): New function. Search for debug file in known + paths. + (open_debugfile_by_gnulink): New function. Open debug file with + gnulink. + (hex): New function. Convert to hex. + (get_build_id_name): New function. Generate build-id name. + (open_debugfile_by_build_id): New function. Open debug file with + build-id. + (backtrace_open_debugfile): New function. Open debug file. + (elf_add): Move code which reads elf header to elf_header_is_valid. + (phdr_callback): Call backtrace_open_debugfile function for shared + library. + * fileline.c (fileline_initialize): Call backtrace_open_debugfile for + executable. + * internal.h: Updated. + 2017-03-08 Sam Thursfield <sam.thursfi...@codethink.co.uk> * btest.c (test5): Replace #ifdef guard with 'unused' attribute diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in index de74b5d..e604b6c 100644 --- a/libbacktrace/Makefile.in +++ b/libbacktrace/Makefile.in @@ -16,7 +16,7 @@ @SET_MAKE@ # Makefile.am -- Backtrace Makefile. -# Copyright (C) 2012-2016 Free Software Foundation, Inc. +# Copyright (C) 2012-2017 Free Software Foundation, Inc. # Redistribution and use in source and binary forms, with or without # modification, are permitted provided that the following conditions are diff --git a/libbacktrace/config.h.in b/libbacktrace/config.h.in index 87cb805..edbd9af 100644 --- a/libbacktrace/config.h.in +++ b/libbacktrace/config.h.in @@ -28,6 +28,9 @@ /* Define to 1 if you have the header file. */ #undef HAVE_INTTYPES_H +/* Define 1 if is available. */ +#undef HAVE_LIMITS_H + /* Define to 1 if you have the header file. */ #undef HAVE_LINK_H @@ -52,6 +55,9
Re: [PATCH][PR sanitizer/80414] Fix segfault with -fsanitize=undefined on 32 bit host
Thanks for review.
I updated the patch.
On 04/13/2017 04:10 PM, Jakub Jelinek wrote:
On Thu, Apr 13, 2017 at 12:28:40PM +0300, Denis Khalikov wrote:
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-15.c
@@ -0,0 +1,11 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+int main()
+{
+ long long offset = 10;
+ char array[10];
+ char c = array[offset];
+ return 0;
+}
I would expect you want to dg-output here the runtime diagnostics,
at least some part of it, to make it clear the testcase is UB and
to test whether the UB is detected.
diff --git a/gcc/ubsan.c b/gcc/ubsan.c
index c01d633..936 100644
--- a/gcc/ubsan.c
+++ b/gcc/ubsan.c
@@ -672,7 +672,8 @@ ubsan_expand_bounds_ifn (gimple_stmt_iterator *gsi)
/* Pick up the arguments of the UBSAN_BOUNDS call. */
tree type = TREE_TYPE (TREE_TYPE (gimple_call_arg (stmt, 0)));
- tree index = gimple_call_arg (stmt, 1);
+ tree index, orig_index;
+ index = orig_index = gimple_call_arg (stmt, 1);
tree orig_index_type = TREE_TYPE (index);
Instead of this I'd suggest:
tree index = gimple_call_arg (stmt, 1);
- tree orig_index_type = TREE_TYPE (index);
+ tree orig_index = index;
tree bound = gimple_call_arg (stmt, 2);
@@ -708,9 +709,9 @@ ubsan_expand_bounds_ifn (gimple_stmt_iterator *gsi)
? BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS
: BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS_ABORT;
tree fn = builtin_decl_explicit (bcode);
- tree val = force_gimple_operand_gsi (gsi, ubsan_encode_value (index),
- true, NULL_TREE, true,
- GSI_SAME_STMT);
+ tree val
+ = force_gimple_operand_gsi (gsi, ubsan_encode_value (orig_index), true,
+ NULL_TREE, true, GSI_SAME_STMT);
g = gimple_build_call (fn, 2, data, val);
}
gimple_set_location (g, loc);
and replace orig_index_type use with TREE_TYPE (orig_index)
Jakub
commit 5267088b655febb8dd9b675e5da7263ada41ead4
Author: Denis Khalikov <d.khali...@partner.samsung.com>
Date: Thu Apr 13 12:03:19 2017 +0300
PR sanitizer/80414
* ubsan.c (ubsan_expand_bounds_ifn): Fix wrong tree val generation
for 32 bit host.
* c-c++-common/ubsan/bounds-15.c: New test.
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 3154103..283dbd6 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2017-04-13 Denis Khalikov <d.khali...@partner.samsung.com>
+
+ PR sanitizer/80414
+ * ubsan.c (ubsan_expand_bounds_ifn): Fix wrong tree val generation
+ for 32 bit host.
+
2017-04-12 Jan Hubicka <hubi...@ucw.cz>
PR lto/69953
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index b1594f2..fe55233 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2017-04-13 Denis Khalikov <d.khali...@partner.samsung.com>
+
+ PR sanitizer/80414
+ * c-c++-common/ubsan/bounds-15.c: New test.
+
2017-04-12 Jakub Jelinek <ja...@redhat.com>
PR tree-optimization/79390
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-15.c b/gcc/testsuite/c-c++-common/ubsan/bounds-15.c
new file mode 100644
index 000..d62f5d5
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-15.c
@@ -0,0 +1,13 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+int main()
+{
+ long long offset = 10;
+ char array[10];
+ char c = array[offset];
+ return 0;
+}
+
+/* { dg-output "\[^\n\r]*index 10 out of bounds for type 'char \\\[10\\\]'" } */
diff --git a/gcc/ubsan.c b/gcc/ubsan.c
index c01d633..4159cc5 100644
--- a/gcc/ubsan.c
+++ b/gcc/ubsan.c
@@ -673,7 +673,7 @@ ubsan_expand_bounds_ifn (gimple_stmt_iterator *gsi)
/* Pick up the arguments of the UBSAN_BOUNDS call. */
tree type = TREE_TYPE (TREE_TYPE (gimple_call_arg (stmt, 0)));
tree index = gimple_call_arg (stmt, 1);
- tree orig_index_type = TREE_TYPE (index);
+ tree orig_index = index;
tree bound = gimple_call_arg (stmt, 2);
gimple_stmt_iterator gsi_orig = *gsi;
@@ -700,7 +700,7 @@ ubsan_expand_bounds_ifn (gimple_stmt_iterator *gsi)
tree data
= ubsan_create_data ("__ubsan_out_of_bounds_data", 1, ,
ubsan_type_descriptor (type, UBSAN_PRINT_ARRAY),
- ubsan_type_descriptor (orig_index_type),
+ ubsan_type_descriptor (TREE_TYPE (orig_index)),
NULL_TREE, NULL_TREE);
data = build_fold_addr_expr_loc (loc, data);
enum built_in_function bcode
@@ -708,9 +708,9 @@ ubsan_expand_bounds_ifn (gimple_stmt_iterator *gsi)
? BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS
: BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS_ABORT;
tree fn = builtin_decl_explicit (bcode);
- tree val = force_gimple_operand_gsi (gsi, ubsa
[PATCH][PR sanitizer/80414] Fix segfault with -fsanitize=undefined on 32 bit host
Hello everyone.
I have patch to fix segfault with -fsanitize=undefined on 32 bit host.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80414
Can someone please review it.
Thanks.
commit 3bb53510ae11a9fa1f79ae83469c2650abe81ab4
Author: Denis Khalikov <d.khali...@partner.samsung.com>
Date: Thu Apr 13 12:03:19 2017 +0300
PR sanitizer/80414
* ubsan.c (ubsan_expand_bounds_ifn): Fix wrong tree val generation
for 32 bit host.
* c-c++-common/ubsan/bounds-15.c: New test.
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 3154103..283dbd6 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2017-04-13 Denis Khalikov <d.khali...@partner.samsung.com>
+
+ PR sanitizer/80414
+ * ubsan.c (ubsan_expand_bounds_ifn): Fix wrong tree val generation
+ for 32 bit host.
+
2017-04-12 Jan Hubicka <hubi...@ucw.cz>
PR lto/69953
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index b1594f2..fe55233 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2017-04-13 Denis Khalikov <d.khali...@partner.samsung.com>
+
+ PR sanitizer/80414
+ * c-c++-common/ubsan/bounds-15.c: New test.
+
2017-04-12 Jakub Jelinek <ja...@redhat.com>
PR tree-optimization/79390
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-15.c b/gcc/testsuite/c-c++-common/ubsan/bounds-15.c
new file mode 100644
index 000..2af709a
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-15.c
@@ -0,0 +1,11 @@
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+int main()
+{
+ long long offset = 10;
+ char array[10];
+ char c = array[offset];
+ return 0;
+}
diff --git a/gcc/ubsan.c b/gcc/ubsan.c
index c01d633..936 100644
--- a/gcc/ubsan.c
+++ b/gcc/ubsan.c
@@ -672,7 +672,8 @@ ubsan_expand_bounds_ifn (gimple_stmt_iterator *gsi)
/* Pick up the arguments of the UBSAN_BOUNDS call. */
tree type = TREE_TYPE (TREE_TYPE (gimple_call_arg (stmt, 0)));
- tree index = gimple_call_arg (stmt, 1);
+ tree index, orig_index;
+ index = orig_index = gimple_call_arg (stmt, 1);
tree orig_index_type = TREE_TYPE (index);
tree bound = gimple_call_arg (stmt, 2);
@@ -708,9 +709,9 @@ ubsan_expand_bounds_ifn (gimple_stmt_iterator *gsi)
? BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS
: BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS_ABORT;
tree fn = builtin_decl_explicit (bcode);
- tree val = force_gimple_operand_gsi (gsi, ubsan_encode_value (index),
- true, NULL_TREE, true,
- GSI_SAME_STMT);
+ tree val
+ = force_gimple_operand_gsi (gsi, ubsan_encode_value (orig_index), true,
+NULL_TREE, true, GSI_SAME_STMT);
g = gimple_build_call (fn, 2, data, val);
}
gimple_set_location (g, loc);
[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello everyone, this is a ping for https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01171.html
[PING][PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello everyone, can someone please review that patch https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01171.html Thanks.
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello everyone, I've fixed some issues and implemented functionality to search debug file by build-id. Can someone please review my patch. > As far as I know all the debuglink code is ELF-specific. I would do > it all in elf.c. While reading the sections of the executable, look > for a debuglink section, and use it if present. Keep the readlink > code in posix.c, I suppose. Apologies if this doesn't make sense. I've moved backtrace_open_debugfile() function into elf.c. This function checks debug sections (build-id/debuglink), and if one of them exist - the function starts to search and open debug files. Also I'll be very appreciated if someone give me advise about issues: 1. | Skimming over the patch I noticed you duplicate libiberties xcrc32 | functionality. To verify that debug file is valid, we should verify crc32 sum, I took that algorithm from gdb, and put it in the libbacktrace sources. However this functionality is implemented by libiberty. In case if libbactrace is being built as a static library, I can't just link libbacktrace against static libbiberty. Libtool can do it with objects which has "la" suffix, but in time when libbacktrace build process happens libiberty.la already removed. Should i extract *.o files from the libiberty archive and create libbacktrace archive with crc32.o from libiberty or just keep this code in libbacktrace ? 2. > > +clean_separate:glinktest.debug > > + rm -f glinktest.debug > > + > > +separate: glinktest > > + if test -n "$(OBJCOPY)" && test -n "$(STRIP)"; \ > > + then \ > > + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\ > > + $(STRIP) glinktest;\ > > + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\ > > + fi; > > |As far as I know "separate" is not a special thing in automake. We > | should always run (and clean) this test if the necessary objcopy > | option is available. > In the attached patch I overrode default target check-TESTS to use objcopy and strip, before test suite will run. But the solution to move all test suite code from Makefile.in to Makefile.am seems not really good. I searched for default target, which can be run before test suite logic, but can't find it for Makefile.am. Should I keep current solution, or we have better alternative? Thanks. On 03/14/2017 04:49 PM, Ian Lance Taylor wrote: On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov <d.khali...@partner.samsung.com> wrote: Hello everyone, i have a patch for this issue. List of implemented functionality: 1.Reading .gnu_debuglink section from ELF file: a. Reading name of debug info file. b. Verifying crc32 sum. 2. Searching for separate debug info file from paths: a. /usr/lib/debug/path/to/executable b. /path/to/executable c. /path/to/executable/.debug Assumed that debug info file generated by objcopy from binutils. objcopy --only-keep-debug foo foo.debug strip -g foo objcopy --add-gnu-debuglink=foo.debug foo +clean_separate:glinktest.debug + rm -f glinktest.debug + +separate: glinktest + if test -n "$(OBJCOPY)" && test -n "$(STRIP)"; \ + then \ + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\ + $(STRIP) glinktest;\ + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\ + fi; As far as I know "separate" is not a special thing in automake. We should always run (and clean) this test if the necessary objcopy option is available. +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h Missing '$' in "$(INCDIR)". +/* Return 1 if header is valid and -1 on fail */ This comment does not explain what the function actually does. +/* Return the pointer to char array with data from .gnudebuglink section inside. */ Line too long, we use 80 column lines. +unsigned char * +elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor, + backtrace_error_callback error_callback, void *data, + int exe, int *gnulink_data_len_out) This should be static. I see that you are calling it elsewhere, but it doesn't make sense to call an "elf" function outside of elf.c. This library is used on non-ELF systems. + /* Look for for the .gnu_debuglink section */ Period at end of sentence. /* To translate PC to file/line when using DWARF, we need to find - the .debug_info and .debug_line sections. */ + the .debug_info and .debug_line sections. */ Why change the indentation like this? I think the original was correct. - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, - pd->data, _not_exist); + descriptor + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, +_does_not_exist, pd->state, 0); + if (descriptor < 0) + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, + pd-
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Thanks for answer, i got it now. I will also delete all readlink code. Looks like is no reason to use it instead just one call of realpath(char*,char*). Binutils using realpath in the same cases. On 03/14/2017 07:26 PM, Ian Lance Taylor wrote: On Tue, Mar 14, 2017 at 7:30 AM, Denis Khalikov <d.khali...@partner.samsung.com> wrote: Thanks for review, got all of my mistakes, except one. - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, - pd->data, _not_exist); + descriptor + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, +_does_not_exist, pd->state, 0); + if (descriptor < 0) + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, + pd->data, _not_exist); This seems like unnecessary work. Shouldn't we only try to open the debug file if we find a .gnu_debuglink section? Should i move code below /* check if debug section does exist */ debug_link_data = elf_gnu_debuglink_section (state, descriptor, error_callbackdata, exe, _link_data_len); from backtrace_open_debugfile . As far as I know all the debuglink code is ELF-specific. I would do it all in elf.c. While reading the sections of the executable, look for a debuglink section, and use it if present. Keep the readlink code in posix.c, I suppose. Apologies if this doesn't make sense. Ian On 03/14/2017 04:49 PM, Ian Lance Taylor wrote: On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov <d.khali...@partner.samsung.com> wrote: Hello everyone, i have a patch for this issue. List of implemented functionality: 1.Reading .gnu_debuglink section from ELF file: a. Reading name of debug info file. b. Verifying crc32 sum. 2. Searching for separate debug info file from paths: a. /usr/lib/debug/path/to/executable b. /path/to/executable c. /path/to/executable/.debug Assumed that debug info file generated by objcopy from binutils. objcopy --only-keep-debug foo foo.debug strip -g foo objcopy --add-gnu-debuglink=foo.debug foo +clean_separate:glinktest.debug + rm -f glinktest.debug + +separate: glinktest + if test -n "$(OBJCOPY)" && test -n "$(STRIP)"; \ + then \ + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\ + $(STRIP) glinktest;\ + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\ + fi; As far as I know "separate" is not a special thing in automake. We should always run (and clean) this test if the necessary objcopy option is available. +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h Missing '$' in "$(INCDIR)". +/* Return 1 if header is valid and -1 on fail */ This comment does not explain what the function actually does. +/* Return the pointer to char array with data from .gnudebuglink section inside. */ Line too long, we use 80 column lines. +unsigned char * +elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor, + backtrace_error_callback error_callback, void *data, + int exe, int *gnulink_data_len_out) This should be static. I see that you are calling it elsewhere, but it doesn't make sense to call an "elf" function outside of elf.c. This library is used on non-ELF systems. + /* Look for for the .gnu_debuglink section */ Period at end of sentence. /* To translate PC to file/line when using DWARF, we need to find - the .debug_info and .debug_line sections. */ + the .debug_info and .debug_line sections. */ Why change the indentation like this? I think the original was correct. - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, - pd->data, _not_exist); + descriptor + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, +_does_not_exist, pd->state, 0); + if (descriptor < 0) + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, + pd->data, _not_exist); This seems like unnecessary work. Shouldn't we only try to open the debug file if we find a .gnu_debuglink section? +/*Just a simple test copied from btest.c, but in this case we don't have debug + * info in the executable and test should verify that we can read debug info + * from separate file. See Makefile check-TESTS target. */ No leading '*' on subsequent lines of multi-line comments, see existing code. I'm not sure I see the point of glinktest.c. Why don't we just use btest.c? +#define MAX_PATH_LEN 4096 /* from linux/limits.h */ This library works on systems other than GNU/Linux. We should dynamically allocate the buffer. + while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1 while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1])) or just call basename. I'm not sure why you bother with the count variable; doesn't full_filename_len - count exactly == len? + sign_byte = 0x00; + count = 0; + while (*(buffer + count) != sign_byte && size > count) +++count; + return count; This looks like `return strnlen(buffer, size)`. Ian
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Thanks for review, got all of my mistakes, except one. > - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, > - pd->data, _not_exist); > + descriptor > + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, > +_does_not_exist, pd->state, 0); > + if (descriptor < 0) > + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, > + pd->data, _not_exist); This seems like unnecessary work. Shouldn't we only try to open the debug file if we find a .gnu_debuglink section? Should i move code below /* check if debug section does exist */ debug_link_data = elf_gnu_debuglink_section (state, descriptor, error_callbackdata, exe, _link_data_len); from backtrace_open_debugfile . On 03/14/2017 04:49 PM, Ian Lance Taylor wrote: On Mon, Mar 13, 2017 at 10:16 AM, Denis Khalikov <d.khali...@partner.samsung.com> wrote: Hello everyone, i have a patch for this issue. List of implemented functionality: 1.Reading .gnu_debuglink section from ELF file: a. Reading name of debug info file. b. Verifying crc32 sum. 2. Searching for separate debug info file from paths: a. /usr/lib/debug/path/to/executable b. /path/to/executable c. /path/to/executable/.debug Assumed that debug info file generated by objcopy from binutils. objcopy --only-keep-debug foo foo.debug strip -g foo objcopy --add-gnu-debuglink=foo.debug foo +clean_separate:glinktest.debug + rm -f glinktest.debug + +separate: glinktest + if test -n "$(OBJCOPY)" && test -n "$(STRIP)"; \ + then \ + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\ + $(STRIP) glinktest;\ + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\ + fi; As far as I know "separate" is not a special thing in automake. We should always run (and clean) this test if the necessary objcopy option is available. +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h Missing '$' in "$(INCDIR)". +/* Return 1 if header is valid and -1 on fail */ This comment does not explain what the function actually does. +/* Return the pointer to char array with data from .gnudebuglink section inside. */ Line too long, we use 80 column lines. +unsigned char * +elf_gnu_debuglink_section (struct backtrace_state *state, int descriptor, + backtrace_error_callback error_callback, void *data, + int exe, int *gnulink_data_len_out) This should be static. I see that you are calling it elsewhere, but it doesn't make sense to call an "elf" function outside of elf.c. This library is used on non-ELF systems. + /* Look for for the .gnu_debuglink section */ Period at end of sentence. /* To translate PC to file/line when using DWARF, we need to find - the .debug_info and .debug_line sections. */ + the .debug_info and .debug_line sections. */ Why change the indentation like this? I think the original was correct. - descriptor = backtrace_open (info->dlpi_name, pd->error_callback, - pd->data, _not_exist); + descriptor + = backtrace_open_debugfile (info->dlpi_name, pd->error_callback, pd->data, +_does_not_exist, pd->state, 0); + if (descriptor < 0) + descriptor = backtrace_open (info->dlpi_name, pd->error_callback, + pd->data, _not_exist); This seems like unnecessary work. Shouldn't we only try to open the debug file if we find a .gnu_debuglink section? +/*Just a simple test copied from btest.c, but in this case we don't have debug + * info in the executable and test should verify that we can read debug info + * from separate file. See Makefile check-TESTS target. */ No leading '*' on subsequent lines of multi-line comments, see existing code. I'm not sure I see the point of glinktest.c. Why don't we just use btest.c? +#define MAX_PATH_LEN 4096 /* from linux/limits.h */ This library works on systems other than GNU/Linux. We should dynamically allocate the buffer. + while (len > 1 && !IS_DIR_SEPARATOR (*(buffer + (len - 1 while (len > 1 && !IS_DIR_SEPARATOR(buffer[len-1])) or just call basename. I'm not sure why you bother with the count variable; doesn't full_filename_len - count exactly == len? + sign_byte = 0x00; + count = 0; + while (*(buffer + count) != sign_byte && size > count) +++count; + return count; This looks like `return strnlen(buffer, size)`. Ian
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Ok, thanks, i will change patch to use crc32 from libiberty and also implement searching for debuginfo with build id. As it was implemented to binutils PR binutils/20876. On 03/14/2017 04:21 PM, Ian Lance Taylor wrote: On Tue, Mar 14, 2017 at 3:20 AM, Denis Khalikov <d.khali...@partner.samsung.com> wrote: Thanks for review, Skimming over the patch I noticed you duplicate libiberties xcrc32 functionality. should i take care about standalone libbacktrace ? https://github.com/ianlancetaylor/libbacktrace No, don't worry about it. Ian
Re: [PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Thanks for review, > Skimming over the patch I noticed you duplicate libiberties xcrc32 > functionality. should i take care about standalone libbacktrace ? https://github.com/ianlancetaylor/libbacktrace > Also the additions to posix.c probably belong to dwarf.c and elf.c (the feature > is dwarf + elf specific but proper abstraction / #ifdefing should > ensure compiling > also succeeds for non-dwarf / non-elf platforms). thanks, i will move code to elf.c since it's about reading section from elf format. On 03/14/2017 11:27 AM, Richard Biener wrote: On Mon, Mar 13, 2017 at 6:16 PM, Denis Khalikov <d.khali...@partner.samsung.com> wrote: Hello everyone, i have a patch for this issue. Great! List of implemented functionality: 1.Reading .gnu_debuglink section from ELF file: a. Reading name of debug info file. b. Verifying crc32 sum. 2. Searching for separate debug info file from paths: a. /usr/lib/debug/path/to/executable b. /path/to/executable c. /path/to/executable/.debug Assumed that debug info file generated by objcopy from binutils. objcopy --only-keep-debug foo foo.debug strip -g foo objcopy --add-gnu-debuglink=foo.debug foo Skimming over the patch I noticed you duplicate libiberties xcrc32 functionality. Also the additions to posix.c probably belong to dwarf.c and elf.c (the feature is dwarf + elf specific but proper abstraction / #ifdefing should ensure compiling also succeeds for non-dwarf / non-elf platforms). Leaving actual review to the maintainer (CCed). Thanks, Richard.
[PATCH][PR sanitizer/77631] Support separate debug info in libbacktrace
Hello everyone, i have a patch for this issue. List of implemented functionality: 1.Reading .gnu_debuglink section from ELF file: a. Reading name of debug info file. b. Verifying crc32 sum. 2. Searching for separate debug info file from paths: a. /usr/lib/debug/path/to/executable b. /path/to/executable c. /path/to/executable/.debug Assumed that debug info file generated by objcopy from binutils. objcopy --only-keep-debug foo foo.debug strip -g foo objcopy --add-gnu-debuglink=foo.debug foo commit 6147ee1a9aeeb748563a8998033f2ce195460162 Author: Denis Khalikov <d.khali...@partner.samsung.com> Date: Mon Mar 13 18:55:36 2017 +0300 PR sanitizer/77631 * Makefile.am: Update to support glinktest. * Makefile.in: Regenerated. * configure.ac: Add AC_CHECK_PROG for objcopy. * configure: Regenerated. * elf.c (elf_header_is_valid): New function. Verify elf header. (elf_add): Move code which reads elf header to elf_header_is_valid. (elf_gnu_debuglink_section): New function. Read debuglink section from elf. (phdr_callback): Call backtrace_open_debugfile function for shared library. * fileline.c (fileline_initialize): Call backtrace_open_debugfile function for executable. * glinktest.c: New test. * internal.h (MAX_PATH_LEN): Defined new variable. * posix.c (enum type_of_file): New enum. (enum debug_path): New enum. (getl32): New function. (gnu_debuglink_crc32): New function. Generate crc32 sum. (get_crc32): New function. (pathlen): New function. (check_sum): New function. Verify sum. (get_debug_filename_len): New function. (backtrace_readlink): New function. (backtrace_open_debugfile): New function. diff --git a/libbacktrace/ChangeLog b/libbacktrace/ChangeLog index 25cd921..cec5d3f 100644 --- a/libbacktrace/ChangeLog +++ b/libbacktrace/ChangeLog @@ -1,3 +1,30 @@ +2017-03-13 Denis Khalikov <d.khali...@partner.samsung.com> + + PR sanitizer/77631 + * Makefile.am: Update to support glinktest. + * Makefile.in: Regenerated. + * configure: Add script to find objcopy in the system. + * elf.c (elf_header_is_valid): New function. Verify elf header. + (elf_add): Move code which reads elf header to elf_header_is_valid. + (elf_gnu_debuglink_section): New function. Read debuglink section from +elf. + (phdr_callback): Call backtrace_open_debugfile function for shared +library. + * fileline.c (fileline_initialize): Call backtrace_open_debugfile +function for executable. + * glinktest.c: New test. + * internal.h (MAX_PATH_LEN): Defined new variable. + * posix.c (enum type_of_file): New enum. + (enum debug_path): New enum. + (getl32): New function. + (gnu_debuglink_crc32): New function. Generate crc32 sum. + (get_crc32): New function. + (pathlen): New function. + (check_sum): New function. Verify sum. + (get_debug_filename_len): New function. + (backtrace_readlink): New function. + (backtrace_open_debugfile): New function. + 2017-03-08 Sam Thursfield <sam.thursfi...@codethink.co.uk> * btest.c (test5): Replace #ifdef guard with 'unused' attribute diff --git a/libbacktrace/Makefile.am b/libbacktrace/Makefile.am index 344dad5..17460a5 100644 --- a/libbacktrace/Makefile.am +++ b/libbacktrace/Makefile.am @@ -81,6 +81,17 @@ libbacktrace_la_LIBADD = \ libbacktrace_la_DEPENDENCIES = $(libbacktrace_la_LIBADD) +clean_separate:glinktest.debug + rm -f glinktest.debug + +separate: glinktest + if test -n "$(OBJCOPY)" && test -n "$(STRIP)"; \ + then \ + $(OBJCOPY) --only-keep-debug glinktest glinktest.debug;\ + $(STRIP) glinktest;\ + $(OBJCOPY) --add-gnu-debuglink=glinktest.debug glinktest;\ + fi; + # Testsuite. check_PROGRAMS = @@ -100,6 +111,12 @@ stest_LDADD = libbacktrace.la check_PROGRAMS += stest +glinktest_SOURCES = glinktest.c +glinktest_CFLAGS = $(AM_CFLAGS) -g -O +glinktest_LDADD = libbacktrace.la + +check_PROGRAMS += glinktest + endif NATIVE # We can't use automake's automatic dependency tracking, because it @@ -134,3 +151,4 @@ sort.lo: config.h backtrace.h internal.h stest.lo: config.h backtrace.h internal.h state.lo: config.h backtrace.h backtrace-supported.h internal.h unknown.lo: config.h backtrace.h internal.h +glinktest.lo: (INCDIR)/filenames.h backtrace.h backtrace-supported.h diff --git a/libbacktrace/Makefile.in b/libbacktrace/Makefile.in index de74b5d..354ecc7 100644 --- a/libbacktrace/Makefile.in +++ b/libbacktrace/Makefile.in @@ -84,7 +84,7 @@ build_triplet = @build@ host_triplet = @host@ target_triplet = @target@ check_PROGRAMS = $(am__EXEEXT_1) -@NATIVE_TRUE@am__append_1 = btest stest +@NATIVE_TRUE@am__append_1 = btest stest glinktest subdi
