[FFmpeg-devel,2/2] configure: disable stripping and memory_poisoning in various toolchains

Submitted by Clément Bœsch on April 9, 2017, 4:46 p.m.

Details

Message ID 20170409164651.31151-2-u@pkh.me
State New
Headers show

Commit Message

Clément Bœsch April 9, 2017, 4:46 p.m.
Toolchains which target debugging are meaningless with stripping and
toolchains which target memory tracking are meaningless with memory
pollution.
---
 configure | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Michael Niedermayer April 10, 2017, 1:51 a.m.
On Sun, Apr 09, 2017 at 06:46:51PM +0200, Clément Bœsch wrote:
> Toolchains which target debugging are meaningless with stripping and
> toolchains which target memory tracking are meaningless with memory
> pollution.
> ---
>  configure | 8 ++++++++
>  1 file changed, 8 insertions(+)

memory_poisoning is not enabled by default in configure

this change would only change the case where the user of configure
enabled explicitly memory_poisoning
IMHO this feels wrong

[...]
Clément Bœsch April 10, 2017, 5:54 a.m.
On Mon, Apr 10, 2017 at 03:51:40AM +0200, Michael Niedermayer wrote:
> On Sun, Apr 09, 2017 at 06:46:51PM +0200, Clément Bœsch wrote:
> > Toolchains which target debugging are meaningless with stripping and
> > toolchains which target memory tracking are meaningless with memory
> > pollution.
> > ---
> >  configure | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> 
> memory_poisoning is not enabled by default in configure
> 

but it is enabled by the fate configure, so if you create a FATE instance
with valgrind, you may forget that memory-poisoning is enabled and make it
useless for uninitialized memory without realizing.

[...]
wm4 April 10, 2017, 6:53 a.m.
On Mon, 10 Apr 2017 07:54:41 +0200
Clément Bœsch <u@pkh.me> wrote:

> On Mon, Apr 10, 2017 at 03:51:40AM +0200, Michael Niedermayer wrote:
> > On Sun, Apr 09, 2017 at 06:46:51PM +0200, Clément Bœsch wrote:  
> > > Toolchains which target debugging are meaningless with stripping and
> > > toolchains which target memory tracking are meaningless with memory
> > > pollution.
> > > ---
> > >  configure | 8 ++++++++
> > >  1 file changed, 8 insertions(+)  
> > 
> > memory_poisoning is not enabled by default in configure
> >   
> 
> but it is enabled by the fate configure, so if you create a FATE instance
> with valgrind, you may forget that memory-poisoning is enabled and make it
> useless for uninitialized memory without realizing.

I love it when such questionable debugging features interfere with real
debugging tools.
Clément Bœsch April 10, 2017, 7:28 a.m.
On Mon, Apr 10, 2017 at 08:53:09AM +0200, wm4 wrote:
> On Mon, 10 Apr 2017 07:54:41 +0200
> Clément Bœsch <u@pkh.me> wrote:
> 
> > On Mon, Apr 10, 2017 at 03:51:40AM +0200, Michael Niedermayer wrote:
> > > On Sun, Apr 09, 2017 at 06:46:51PM +0200, Clément Bœsch wrote:  
> > > > Toolchains which target debugging are meaningless with stripping and
> > > > toolchains which target memory tracking are meaningless with memory
> > > > pollution.
> > > > ---
> > > >  configure | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)  
> > > 
> > > memory_poisoning is not enabled by default in configure
> > >   
> > 
> > but it is enabled by the fate configure, so if you create a FATE instance
> > with valgrind, you may forget that memory-poisoning is enabled and make it
> > useless for uninitialized memory without realizing.
> 
> I love it when such questionable debugging features interfere with real
> debugging tools.

Well, it's a debugging "tool". This is another debate, but if people
prefer that I drop the feature, it's fine with me. I introduced it a while
ago believing it would help spotting simple cases of uninitialized memory.
I don't know if it had any practical use cases yet.
Hendrik Leppkes April 10, 2017, 7:46 a.m.
On Mon, Apr 10, 2017 at 8:53 AM, wm4 <nfxjfg@googlemail.com> wrote:
> On Mon, 10 Apr 2017 07:54:41 +0200
> Clément Bœsch <u@pkh.me> wrote:
>
>> On Mon, Apr 10, 2017 at 03:51:40AM +0200, Michael Niedermayer wrote:
>> > On Sun, Apr 09, 2017 at 06:46:51PM +0200, Clément Bœsch wrote:
>> > > Toolchains which target debugging are meaningless with stripping and
>> > > toolchains which target memory tracking are meaningless with memory
>> > > pollution.
>> > > ---
>> > >  configure | 8 ++++++++
>> > >  1 file changed, 8 insertions(+)
>> >
>> > memory_poisoning is not enabled by default in configure
>> >
>>
>> but it is enabled by the fate configure, so if you create a FATE instance
>> with valgrind, you may forget that memory-poisoning is enabled and make it
>> useless for uninitialized memory without realizing.
>
> I love it when such questionable debugging features interfere with real
> debugging tools.

Memory poisoning is quite useful, really, and its just a configure
flag like any other, disabled by default.
IMHO, if someone sets up a valgrind box, its his job to remember to
turn this off. configure should not disable explicitly set configure
flags.

- Hendrik
wm4 April 10, 2017, 7:51 a.m.
On Mon, 10 Apr 2017 09:28:06 +0200
Clément Bœsch <u@pkh.me> wrote:

> On Mon, Apr 10, 2017 at 08:53:09AM +0200, wm4 wrote:
> > On Mon, 10 Apr 2017 07:54:41 +0200
> > Clément Bœsch <u@pkh.me> wrote:
> >   
> > > On Mon, Apr 10, 2017 at 03:51:40AM +0200, Michael Niedermayer wrote:  
> > > > On Sun, Apr 09, 2017 at 06:46:51PM +0200, Clément Bœsch wrote:    
> > > > > Toolchains which target debugging are meaningless with stripping and
> > > > > toolchains which target memory tracking are meaningless with memory
> > > > > pollution.
> > > > > ---
> > > > >  configure | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)    
> > > > 
> > > > memory_poisoning is not enabled by default in configure
> > > >     
> > > 
> > > but it is enabled by the fate configure, so if you create a FATE instance
> > > with valgrind, you may forget that memory-poisoning is enabled and make it
> > > useless for uninitialized memory without realizing.  
> > 
> > I love it when such questionable debugging features interfere with real
> > debugging tools.  
> 
> Well, it's a debugging "tool". This is another debate, but if people
> prefer that I drop the feature, it's fine with me. I introduced it a while
> ago believing it would help spotting simple cases of uninitialized memory.
> I don't know if it had any practical use cases yet.
> 

I'd argue against the feature, because the same can be achieved with
LD_PRELOADING something that replaces malloc.

Patch hide | download patch | download mbox

diff --git a/configure b/configure
index 539a126656..f27ede94d9 100755
--- a/configure
+++ b/configure
@@ -3591,11 +3591,14 @@  case "$toolchain" in
         cc_default="${toolchain%-asan}"
         add_cflags  -fsanitize=address
         add_ldflags -fsanitize=address
+        disable stripping
     ;;
     *-msan)
         cc_default="${toolchain%-msan}"
         add_cflags  -fsanitize=memory -fsanitize-memory-track-origins
         add_ldflags -fsanitize=memory
+        disable stripping
+        disable memory_poisoning
     ;;
     *-tsan)
         cc_default="${toolchain%-tsan}"
@@ -3607,11 +3610,13 @@  case "$toolchain" in
                 add_ldflags -fPIC
                 ;;
         esac
+        disable stripping
     ;;
     *-usan)
         cc_default="${toolchain%-usan}"
         add_cflags  -fsanitize=undefined
         add_ldflags -fsanitize=undefined
+        disable stripping
     ;;
     valgrind-*)
         target_exec_default="valgrind"
@@ -3624,6 +3629,8 @@  case "$toolchain" in
                 target_exec_args="--error-exitcode=1 --malloc-fill=0x2a --track-origins=yes --leak-check=full --gen-suppressions=all --suppressions=$source_path/tests/fate-valgrind.supp"
                 ;;
         esac
+        disable stripping
+        disable memory_poisoning
     ;;
     msvc)
         # Check whether the current MSVC version needs the C99 converter.
@@ -3664,6 +3671,7 @@  case "$toolchain" in
     gcov)
         add_cflags  -fprofile-arcs -ftest-coverage
         add_ldflags -fprofile-arcs -ftest-coverage
+        disable stripping
     ;;
     llvm-cov)
         add_cflags -fprofile-arcs -ftest-coverage