configure: add ABIGAIL_DEBUG options

  When exporting ABIGAIL_DEBUG=1, the binaries compiled are especially
suitable for debugging. The CFLAGS and CXXFLAGS that are added disable
optimization and increase debug information levels.

	* configure.ac: add ABIGAIL_DEBUG environment variable for
	improved debugging capabilities

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 configure.ac | 5 +++++
 1 file changed, 5 insertions(+)
  

Hi Matthias,

On Mon, 2020-05-11 at 17:24 +0200, Matthias Maennich via Libabigail wrote:
> When exporting ABIGAIL_DEBUG=1, the binaries compiled are especially
> suitable for debugging. The CFLAGS and CXXFLAGS that are added disable
> optimization and increase debug information levels.
> 
> 	* configure.ac: add ABIGAIL_DEBUG environment variable for
> 	improved debugging capabilities
> 
> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
>  configure.ac | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 9f30ea38cf86..9aea79f49e9a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -621,6 +621,11 @@ if test x$ABIGAIL_DEVEL != x; then
>     CXXFLAGS="-g -Wall -Wextra -Werror"
>  fi
>  
> +if test x$ABIGAIL_DEBUG != x; then
> +    CFLAGS="$CFLAGS -O0 -g3 -ggdb"
> +    CXXFLAGS="$CXXFLAGS -O0 -g3 -ggdb"
> +fi
> +

I think you either want -O0 plus -fno-inline to make debugging even
easier. Or (better IMHO) use -Og so you do get some optimization, just
none that makes debugging harder. The advantage of -Og is that you can
then also add -D_FORTIFY_SOURCE=2 to detect various memory and string
operation bugger overflows early (or even at compile time).

You also will want to add -D_GLIBCXX_DEBUG which catches various
illegal uses of std iterators and algorithms. In fact I just tried it
and while running make check it found:

   safe_iterator.h:360:error: attempt to advance 
       signed char dereferenceable (start-of-sequence) iterator -1
   steps, which falls     
       outside its valid range.

   Objects involved in the operation:
   iterator @ 0x0x7fffffffc740 {
   type =
   __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<abigail::ir
   ::function_decl* const*,
   std::__cxx1998::vector<abigail::ir::function_decl*,
   std::allocator<abigail::ir::function_decl*> > >,
   std::__debug::vector<abigail::ir::function_decl*,
   std::allocator<abigail::ir::function_decl*> > > (constant iterator);
     state = dereferenceable (start-of-sequence);
     references sequence with type
   `std::__debug::vector<abigail::ir::function_decl*,
   std::allocator<abigail::ir::function_decl*> >' @ 0x0x7fffffffc740 }

Poking around in gdb found that in compute_diff () the a_base
RandomAccessOutputIterator could go back (-1) and then forward (+1)
again because there were missing brackets. That is undefined behavior
if we are at the beginning of the iterator.

Proposed fix attached.

Cheers,

Mark
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Don-t-iterate-before-the-start-of-a-RandomAccessOutp.patch
Type: text/x-patch
Size: 1843 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libabigail/attachments/20200511/6124de7e/attachment.bin>
  

Don?t do -O0, that skips some of the compiler passes which are needed to generate good quality DWARF.

> On May 11, 2020, at 8:24 AM, Matthias Maennich via Libabigail <libabigail@sourceware.org> wrote:
> 
> When exporting ABIGAIL_DEBUG=1, the binaries compiled are especially
> suitable for debugging. The CFLAGS and CXXFLAGS that are added disable
> optimization and increase debug information levels.
> 
> 	* configure.ac: add ABIGAIL_DEBUG environment variable for
> 	improved debugging capabilities
> 
> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
> configure.ac | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 9f30ea38cf86..9aea79f49e9a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -621,6 +621,11 @@ if test x$ABIGAIL_DEVEL != x; then
>    CXXFLAGS="-g -Wall -Wextra -Werror"
> fi
> 
> +if test x$ABIGAIL_DEBUG != x; then
> +    CFLAGS="$CFLAGS -O0 -g3 -ggdb"
> +    CXXFLAGS="$CXXFLAGS -O0 -g3 -ggdb"
> +fi
> +
> if test x$ENABLE_ASAN = xyes; then
>     CFLAGS="$CFLAGS -fsanitize=address"
>     CXXFLAGS="$CXXFLAGS -fsanitize=address"
> -- 
> 2.26.2.645.ge9eca65c58-goog
>
  

Hi Mark and Ben!

On Mon, May 11, 2020 at 07:04:34PM +0200, Mark Wielaard wrote:
>Hi Matthias,
>
>On Mon, 2020-05-11 at 17:24 +0200, Matthias Maennich via Libabigail wrote:
>> When exporting ABIGAIL_DEBUG=1, the binaries compiled are especially
>> suitable for debugging. The CFLAGS and CXXFLAGS that are added disable
>> optimization and increase debug information levels.
>>
>> 	* configure.ac: add ABIGAIL_DEBUG environment variable for
>> 	improved debugging capabilities
>>
>> Signed-off-by: Matthias Maennich <maennich@google.com>
>> ---
>>  configure.ac | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 9f30ea38cf86..9aea79f49e9a 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -621,6 +621,11 @@ if test x$ABIGAIL_DEVEL != x; then
>>     CXXFLAGS="-g -Wall -Wextra -Werror"
>>  fi
>>
>> +if test x$ABIGAIL_DEBUG != x; then
>> +    CFLAGS="$CFLAGS -O0 -g3 -ggdb"
>> +    CXXFLAGS="$CXXFLAGS -O0 -g3 -ggdb"
>> +fi
>> +
>
>I think you either want -O0 plus -fno-inline to make debugging even
>easier. Or (better IMHO) use -Og so you do get some optimization, just
>none that makes debugging harder. The advantage of -Og is that you can
>then also add -D_FORTIFY_SOURCE=2 to detect various memory and string
>operation bugger overflows early (or even at compile time).

I am totally for -Og in theory, but had bad experience when using it (as
in incomlete debug info).  But I believe this was in the early days of
-Og (introduced with 4.8?) and I do not remember the details anymore. I
am happy to be convinced that this is now superior for debugging without
downsides and will adjust the patch.

>
>You also will want to add -D_GLIBCXX_DEBUG which catches various

That is indeed a good idea. The define seems more suitable for
ABIGAIL_DEVEL though. When running with this define I can confirm the
bug that you found here as well as:

In function:
     void std::sort(_RandomAccessIterator, _RandomAccessIterator, _Compare)
     [_RandomAccessIterator =
     __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<std::shared_ptr<abigail::ir::type_base>
     *, std::__cxx1998::vector<std::shared_ptr<abigail::ir::type_base>,
     std::allocator<std::shared_ptr<abigail::ir::type_base> > > >,
     std::__debug::vector<std::shared_ptr<abigail::ir::type_base>,
     std::allocator<std::shared_ptr<abigail::ir::type_base> > >,
     std::random_access_iterator_tag>, _Compare =
     abigail::ir::type_topo_comp]

Error: comparison doesn't meet irreflexive requirements, assert(!(a < a)).

Objects involved in the operation:
     instance "functor" @ 0x0x7ffd978f69d8 {
       type = __gnu_debug::_Error_formatter::_Parameter::_Type;
     }
     iterator::value_type "ordered type" {
       type = std::shared_ptr<abigail::ir::type_base>;
     }

The non-trivial comparator seems to be affected by this. I did not have
the time to look into that, but wanted to report it here at least.

>illegal uses of std iterators and algorithms. In fact I just tried it
>and while running make check it found:
>
>   safe_iterator.h:360:error: attempt to advance
>       signed char dereferenceable (start-of-sequence) iterator -1
>   steps, which falls
>       outside its valid range.
>
>   Objects involved in the operation:
>   iterator @ 0x0x7fffffffc740 {
>   type =
>   __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<abigail::ir
>   ::function_decl* const*,
>   std::__cxx1998::vector<abigail::ir::function_decl*,
>   std::allocator<abigail::ir::function_decl*> > >,
>   std::__debug::vector<abigail::ir::function_decl*,
>   std::allocator<abigail::ir::function_decl*> > > (constant iterator);
>     state = dereferenceable (start-of-sequence);
>     references sequence with type
>   `std::__debug::vector<abigail::ir::function_decl*,
>   std::allocator<abigail::ir::function_decl*> >' @ 0x0x7fffffffc740 }
>
>Poking around in gdb found that in compute_diff () the a_base
>RandomAccessOutputIterator could go back (-1) and then forward (+1)
>again because there were missing brackets. That is undefined behavior
>if we are at the beginning of the iterator.
>
>Proposed fix attached.
>
>Cheers,
>
>Mark

>From a5b9bda8a44da06985fbd938b55e209c94893175 Mon Sep 17 00:00:00 2001
>From: Mark Wielaard <mark@klomp.org>
>Date: Mon, 11 May 2020 18:55:03 +0200
>Subject: [PATCH] Don't iterate before the start of a
> RandomAccessOutputIterator.
>
>Found with -D_GLIBCXX_DEBUG. You cannot go before the start of an
>RandomAccessOutputIterator. Iterator -1 + 1 might seem to work,
>but is actually undefined behaviour.
>
>	* include/abg-diff-utils.h (compute_diff): Put brackets around
>	p[ux].[xy]() + 1 calculation.
>
>Signed-off-by: Mark Wielaard <mark@klomp.org>
>---
> include/abg-diff-utils.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/include/abg-diff-utils.h b/include/abg-diff-utils.h
>index 3cbdbf33..8bc667d0 100644
>--- a/include/abg-diff-utils.h
>+++ b/include/abg-diff-utils.h
>@@ -1591,8 +1591,8 @@ compute_diff(RandomAccessOutputIterator a_base,
>       point px, pu;
>       snake_end_points(snak, px, pu);
>       compute_diff<RandomAccessOutputIterator,
>-		   EqualityFunctor>(a_base, a_begin, a_base + px.x() + 1,
>-				    b_base, b_begin, b_base + px.y() + 1,
>+		   EqualityFunctor>(a_base, a_begin, a_base + (px.x() + 1),
>+				    b_base, b_begin, b_base + (px.y() + 1),
> 				    lcs, tmp_ses0, tmp_ses_len0);
> 
>       lcs.insert(lcs.end(), trace.begin(), trace.end());
>@@ -1600,8 +1600,8 @@ compute_diff(RandomAccessOutputIterator a_base,
>       int tmp_ses_len1 = 0;
>       edit_script tmp_ses1;
>       compute_diff<RandomAccessOutputIterator,
>-		   EqualityFunctor>(a_base, a_base + pu.x() + 1, a_end,
>-				    b_base, b_base + pu.y() + 1, b_end,
>+		   EqualityFunctor>(a_base, a_base + (pu.x() + 1), a_end,
>+				    b_base, b_base + (pu.y() + 1), b_end,
> 				    lcs, tmp_ses1, tmp_ses_len1);
>       ABG_ASSERT(tmp_ses0.length() + tmp_ses1.length() == d);
>       ABG_ASSERT(tmp_ses_len0 + tmp_ses_len1 == d);

That looks good to me. And I can confirm this almost fixing `make check`
with with _GLIBCXX_DEBUG set. It remains the above reported one.

Reviewed-by: Matthias Maennich <maennich@google.com>
Tested-by: Matthias Maennich <maennich@google.com>

Cheers,
Matthias

>-- 
>2.18.4
>
  

Hello,

Mark Wielaard <mark@klomp.org> a écrit:

> Hi Matthias,
>
> On Mon, 2020-05-11 at 22:07 +0200, Matthias Maennich wrote:
>> On Mon, May 11, 2020 at 07:04:34PM +0200, Mark Wielaard wrote:
>> > I think you either want -O0 plus -fno-inline to make debugging even
>> > easier. Or (better IMHO) use -Og so you do get some optimization, just
>> > none that makes debugging harder. The advantage of -Og is that you can
>> > then also add -D_FORTIFY_SOURCE=2 to detect various memory and string
>> > operation bugger overflows early (or even at compile time).
>> 
>> I am totally for -Og in theory, but had bad experience when using it (as
>> in incomlete debug info).  But I believe this was in the early days of
>> -Og (introduced with 4.8?) and I do not remember the details anymore. I
>> am happy to be convinced that this is now superior for debugging without
>> downsides and will adjust the patch.
>
> As Ben said without any optimization -O0 gcc doesn't do any
> debug/tracking passes, so for example there is no var-tracking-
> assignments, which means no location descriptions. A debugger mostly
> has to rely on knowing the calling conventions to show you arguments
> and variable values. And you also cannot enable -D_FORTIFY_SOURCE=2
> which I have found very useful (admittedly in C projects, in C++ it
> might have less exposure). Personally I would use -Og plus
> -D_FORTIFY_SOURCE=2. -Og also makes it easier to use some profilers.
> But if you do think even -Og optimizes too much to make debugging
> harder, then I would go the other way and use -O0 -fno-inline which
> helps debugging in my experience (it prevents even private method calls
> to be inlined, which makes the debugger able to just call them
> directly).

I agree, I'd use -Og rather than -OO, and I'd add -D_FORTIFY_SOURCE=2 as
well.

>
>> > You also will want to add -D_GLIBCXX_DEBUG which catches various
>> 
>> That is indeed a good idea. The define seems more suitable for
>> ABIGAIL_DEVEL though.
>
> I don't think it is appropriate for ABIGAIL_DEVEL since that only adds
> compiler warnings, but doesn't change code generation. -D_GLIBCXX_DEBUG
> is more like the compiler sanitizer options, it changes the code (and
> abi).

[...]

Matthias Maennich via Libabigail <libabigail@sourceware.org> a écrit:

> I see ABIGAIL_DEVEL as a setting that somebody developing on libabigail
> should have turned on. As such it falls into that category. But I see
> your point in regards to the ABI. Since I do not use libabigail as a
> library for other than libabigail owned tools, this is not affecting me.
>

Why not just having one ABIGAIL_DEVEL option (and do away with
ABIGAIL_DEBUG) where we pack everything?  By everything, I mean both
what's supposed to be in ABIGAIL_DEBUG and what's already in
ABIGAIL_DEVEL.  I mean, why would someone want to use ABIGAIL_DEBUG
without having the "benefits" of what ABIGAIL_DEVEL provides today, in
practice?

If you guys think strongly that both should be separated, then I'd add
-D_GLIBCXX_DEBUG and -D_FORTIFY_SOURCE=2 to ABIGAIL_DEVEL (in a separate
patch, granted -- I can do that if you like) because I'd like
ABIGAIL_DEVEL to catch as many issues as possible without adding visible
runtime cost for "make check"; that is why the sanitizer options are
separate.

Thanks for this discussion.
  

Hello,

Mark Wielaard <mark@klomp.org> a écrit:

[...]

> From a5b9bda8a44da06985fbd938b55e209c94893175 Mon Sep 17 00:00:00 2001
> From: Mark Wielaard <mark@klomp.org>
> Date: Mon, 11 May 2020 18:55:03 +0200
> Subject: [PATCH] Don't iterate before the start of a
>  RandomAccessOutputIterator.
>
> Found with -D_GLIBCXX_DEBUG. You cannot go before the start of an
> RandomAccessOutputIterator. Iterator -1 + 1 might seem to work,
> but is actually undefined behaviour.
>
> 	* include/abg-diff-utils.h (compute_diff): Put brackets around
> 	p[ux].[xy]() + 1 calculation.
>
> Signed-off-by: Mark Wielaard <mark@klomp.org>
Acked-by: Dodji Seketeli <dodji@seketeli.org>

Applied to master, thanks!
  

>> On Mon, May 11, 2020 at 07:04:34PM +0200, Mark Wielaard wrote:

[...]

>>You also will want to add -D_GLIBCXX_DEBUG which catches various

[...]

Matthias Maennich via Libabigail <libabigail@sourceware.org> a écrit:

> When running with this define I can confirm the
> bug that you found here as well as:
>
> In function:
>     void std::sort(_RandomAccessIterator, _RandomAccessIterator, _Compare)
>     [_RandomAccessIterator =
>     __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<std::shared_ptr<abigail::ir::type_base>
>     *, std::__cxx1998::vector<std::shared_ptr<abigail::ir::type_base>,
>     std::allocator<std::shared_ptr<abigail::ir::type_base> > > >,
>     std::__debug::vector<std::shared_ptr<abigail::ir::type_base>,
>     std::allocator<std::shared_ptr<abigail::ir::type_base> > >,
>     std::random_access_iterator_tag>, _Compare =
>     abigail::ir::type_topo_comp]
>
> Error: comparison doesn't meet irreflexive requirements, assert(!(a < a)).
>
> Objects involved in the operation:
>     instance "functor" @ 0x0x7ffd978f69d8 {
>       type = __gnu_debug::_Error_formatter::_Parameter::_Type;
>     }
>     iterator::value_type "ordered type" {
>       type = std::shared_ptr<abigail::ir::type_base>;
>     }

Indeed, I've seen that as well.  I have applied Mark's patch for that.

> The non-trivial comparator seems to be affected by this.

I don't undertand what you mean by this.

> I did not have the time to look into that, but wanted to report it
> here at least.

Maybe you can just reply with the output you are seeing and I'll file a
bug for it.

Thanks.

Cheers,
  

On Wed, May 13, 2020 at 01:19:59PM +0200, Dodji Seketeli wrote:
>
>>> On Mon, May 11, 2020 at 07:04:34PM +0200, Mark Wielaard wrote:
>
>[...]
>
>>>You also will want to add -D_GLIBCXX_DEBUG which catches various
>
>[...]
>
>Matthias Maennich via Libabigail <libabigail@sourceware.org> a écrit:
>
>> When running with this define I can confirm the
>> bug that you found here as well as:
>>
>> In function:
>>     void std::sort(_RandomAccessIterator, _RandomAccessIterator, _Compare)
>>     [_RandomAccessIterator =
>>     __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<std::shared_ptr<abigail::ir::type_base>
>>     *, std::__cxx1998::vector<std::shared_ptr<abigail::ir::type_base>,
>>     std::allocator<std::shared_ptr<abigail::ir::type_base> > > >,
>>     std::__debug::vector<std::shared_ptr<abigail::ir::type_base>,
>>     std::allocator<std::shared_ptr<abigail::ir::type_base> > >,
>>     std::random_access_iterator_tag>, _Compare =
>>     abigail::ir::type_topo_comp]
>>
>> Error: comparison doesn't meet irreflexive requirements, assert(!(a < a)).
>>
>> Objects involved in the operation:
>>     instance "functor" @ 0x0x7ffd978f69d8 {
>>       type = __gnu_debug::_Error_formatter::_Parameter::_Type;
>>     }
>>     iterator::value_type "ordered type" {
>>       type = std::shared_ptr<abigail::ir::type_base>;
>>     }
>
>Indeed, I've seen that as well.  I have applied Mark's patch for that.
>
>> The non-trivial comparator seems to be affected by this.
>
>I don't undertand what you mean by this.

type_topo_comp implements the comparator that is affected. It is not a
trivial implementation as sometimes for comparators. I believe this is
what I wanted to say :-)

>
>> I did not have the time to look into that, but wanted to report it
>> here at least.
>
>Maybe you can just reply with the output you are seeing and I'll file a
>bug for it.

I filed https://sourceware.org/bugzilla/show_bug.cgi?id=25989 for this.

Cheers,
Matthias

>
>Thanks.
>
>Cheers,
>
>-- 
>		Dodji
  

Hi!

On Wed, May 13, 2020 at 12:43:18PM +0200, Dodji Seketeli wrote:
>Hello,
>
>Mark Wielaard <mark@klomp.org> a écrit:
>
>> Hi Matthias,
>>
>> On Mon, 2020-05-11 at 22:07 +0200, Matthias Maennich wrote:
>>> On Mon, May 11, 2020 at 07:04:34PM +0200, Mark Wielaard wrote:
>>> > I think you either want -O0 plus -fno-inline to make debugging even
>>> > easier. Or (better IMHO) use -Og so you do get some optimization, just
>>> > none that makes debugging harder. The advantage of -Og is that you can
>>> > then also add -D_FORTIFY_SOURCE=2 to detect various memory and string
>>> > operation bugger overflows early (or even at compile time).
>>>
>>> I am totally for -Og in theory, but had bad experience when using it (as
>>> in incomlete debug info).  But I believe this was in the early days of
>>> -Og (introduced with 4.8?) and I do not remember the details anymore. I
>>> am happy to be convinced that this is now superior for debugging without
>>> downsides and will adjust the patch.
>>
>> As Ben said without any optimization -O0 gcc doesn't do any
>> debug/tracking passes, so for example there is no var-tracking-
>> assignments, which means no location descriptions. A debugger mostly
>> has to rely on knowing the calling conventions to show you arguments
>> and variable values. And you also cannot enable -D_FORTIFY_SOURCE=2
>> which I have found very useful (admittedly in C projects, in C++ it
>> might have less exposure). Personally I would use -Og plus
>> -D_FORTIFY_SOURCE=2. -Og also makes it easier to use some profilers.
>> But if you do think even -Og optimizes too much to make debugging
>> harder, then I would go the other way and use -O0 -fno-inline which
>> helps debugging in my experience (it prevents even private method calls
>> to be inlined, which makes the debugger able to just call them
>> directly).
>
>I agree, I'd use -Og rather than -OO, and I'd add -D_FORTIFY_SOURCE=2 as
>well.
>
>>
>>> > You also will want to add -D_GLIBCXX_DEBUG which catches various
>>>
>>> That is indeed a good idea. The define seems more suitable for
>>> ABIGAIL_DEVEL though.
>>
>> I don't think it is appropriate for ABIGAIL_DEVEL since that only adds
>> compiler warnings, but doesn't change code generation. -D_GLIBCXX_DEBUG
>> is more like the compiler sanitizer options, it changes the code (and
>> abi).
>
>[...]
>
>Matthias Maennich via Libabigail <libabigail@sourceware.org> a écrit:
>
>> I see ABIGAIL_DEVEL as a setting that somebody developing on libabigail
>> should have turned on. As such it falls into that category. But I see
>> your point in regards to the ABI. Since I do not use libabigail as a
>> library for other than libabigail owned tools, this is not affecting me.
>>
>
>Why not just having one ABIGAIL_DEVEL option (and do away with
>ABIGAIL_DEBUG) where we pack everything?  By everything, I mean both
>what's supposed to be in ABIGAIL_DEBUG and what's already in
>ABIGAIL_DEVEL.  I mean, why would someone want to use ABIGAIL_DEBUG
>without having the "benefits" of what ABIGAIL_DEVEL provides today, in
>practice?

I think I regularly use it the other way around ABIGAIL_DEVEL without
ABIGAIL_DEBUG. That makes sense as I want all the warnings turned on,
but since I do not want to pay the cost for debuggability (yet), I still
compile -O2.

I never use it the other way around, but since -Wall triggers -Wunused,
that might be sometimes annoying during debug sessions, hence
ABIGAIL_DEBUG without ABIGAIL_DEVEL.

And this is why I split them into two with distinct value.

>
>If you guys think strongly that both should be separated, then I'd add
>-D_GLIBCXX_DEBUG and -D_FORTIFY_SOURCE=2 to ABIGAIL_DEVEL (in a separate
>patch, granted -- I can do that if you like) because I'd like
>ABIGAIL_DEVEL to catch as many issues as possible without adding visible
>runtime cost for "make check"; that is why the sanitizer options are
>separate.

My personal preference would be to add in

ABGAIL_DEVEL
   -D_GLIBCXX_DEBUG
   -D_FORTIFY_SOURCE=2

ABIGAIL_DEBUG
   -Og
   -g3
   -ggdb

Technically we should also run the buildbots in distcheck with
ABIGAIL_DEVEL to catch as much as we can, warnings, runtime checks, etc.

Cheers,
Matthias

>
>Thanks for this discussion.
>
>-- 
>		Dodji
  

Hello,

Matthias Maennich <maennich@google.com> a écrit:

[...]

> My personal preference would be to add in
>
> ABGAIL_DEVEL
>   -D_GLIBCXX_DEBUG
>   -D_FORTIFY_SOURCE=2
>
> ABIGAIL_DEBUG
>   -Og
>   -g3
>   -ggdb

Amen to that, then.

I pre-approve a patch that would do these, unless someone disagrees and
think we should discuss this further.

I can also write the patch if you guys prefer.

Thanks!

Cheers,

> Technically we should also run the buildbots in distcheck with
> ABIGAIL_DEVEL to catch as much as we can, warnings, runtime checks,
> etc.

FWIW, this makes sense to me.
  

On Mon, May 11, 2020 at 10:07:21PM +0200, Matthias Maennich wrote:
>Hi Mark and Ben!
>
>On Mon, May 11, 2020 at 07:04:34PM +0200, Mark Wielaard wrote:
>>Hi Matthias,
>>
>>On Mon, 2020-05-11 at 17:24 +0200, Matthias Maennich via Libabigail wrote:
>>>When exporting ABIGAIL_DEBUG=1, the binaries compiled are especially
>>>suitable for debugging. The CFLAGS and CXXFLAGS that are added disable
>>>optimization and increase debug information levels.
>>>
>>>     * configure.ac: add ABIGAIL_DEBUG environment variable for
>>>     improved debugging capabilities
>>>
>>>Signed-off-by: Matthias Maennich <maennich@google.com>
>>>---
>>> configure.ac | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>>diff --git a/configure.ac b/configure.ac
>>>index 9f30ea38cf86..9aea79f49e9a 100644
>>>--- a/configure.ac
>>>+++ b/configure.ac
>>>@@ -621,6 +621,11 @@ if test x$ABIGAIL_DEVEL != x; then
>>>    CXXFLAGS="-g -Wall -Wextra -Werror"
>>> fi
>>>
>>>+if test x$ABIGAIL_DEBUG != x; then
>>>+    CFLAGS="$CFLAGS -O0 -g3 -ggdb"
>>>+    CXXFLAGS="$CXXFLAGS -O0 -g3 -ggdb"
>>>+fi
>>>+
>>
>>I think you either want -O0 plus -fno-inline to make debugging even
>>easier. Or (better IMHO) use -Og so you do get some optimization, just
>>none that makes debugging harder. The advantage of -Og is that you can
>>then also add -D_FORTIFY_SOURCE=2 to detect various memory and string
>>operation bugger overflows early (or even at compile time).
>
>I am totally for -Og in theory, but had bad experience when using it (as
>in incomlete debug info).  But I believe this was in the early days of
>-Og (introduced with 4.8?) and I do not remember the details anymore. I
>am happy to be convinced that this is now superior for debugging without
>downsides and will adjust the patch.
>

FWIW, I was just debugging with -Og and was experiencing 'value has been
optimized out' in gdb when printing local variables. I recompiled with
-O0 and now those values show up. It seems, Clang 9 (that I am using) has a
different interpretation of -Og or I am missing something else. That
particular case appeared to work with GCC 9.2, though. So, maybe that
is where my initial concerns came from. I am completely on the GCC
side here, hence -Og is still what it should be.

Cheers,
Matthias



>>
>>You also will want to add -D_GLIBCXX_DEBUG which catches various
>
>That is indeed a good idea. The define seems more suitable for
>ABIGAIL_DEVEL though. When running with this define I can confirm the
>bug that you found here as well as:
>
>In function:
>    void std::sort(_RandomAccessIterator, _RandomAccessIterator, _Compare)
>    [_RandomAccessIterator =
>    __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<std::shared_ptr<abigail::ir::type_base>
>    *, std::__cxx1998::vector<std::shared_ptr<abigail::ir::type_base>,
>    std::allocator<std::shared_ptr<abigail::ir::type_base> > > >,
>    std::__debug::vector<std::shared_ptr<abigail::ir::type_base>,
>    std::allocator<std::shared_ptr<abigail::ir::type_base> > >,
>    std::random_access_iterator_tag>, _Compare =
>    abigail::ir::type_topo_comp]
>
>Error: comparison doesn't meet irreflexive requirements, assert(!(a < a)).
>
>Objects involved in the operation:
>    instance "functor" @ 0x0x7ffd978f69d8 {
>      type = __gnu_debug::_Error_formatter::_Parameter::_Type;
>    }
>    iterator::value_type "ordered type" {
>      type = std::shared_ptr<abigail::ir::type_base>;
>    }
>
>The non-trivial comparator seems to be affected by this. I did not have
>the time to look into that, but wanted to report it here at least.
>
>>illegal uses of std iterators and algorithms. In fact I just tried it
>>and while running make check it found:
>>
>>  safe_iterator.h:360:error: attempt to advance
>>      signed char dereferenceable (start-of-sequence) iterator -1
>>  steps, which falls
>>      outside its valid range.
>>
>>  Objects involved in the operation:
>>  iterator @ 0x0x7fffffffc740 {
>>  type =
>>  __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<abigail::ir
>>  ::function_decl* const*,
>>  std::__cxx1998::vector<abigail::ir::function_decl*,
>>  std::allocator<abigail::ir::function_decl*> > >,
>>  std::__debug::vector<abigail::ir::function_decl*,
>>  std::allocator<abigail::ir::function_decl*> > > (constant iterator);
>>    state = dereferenceable (start-of-sequence);
>>    references sequence with type
>>  `std::__debug::vector<abigail::ir::function_decl*,
>>  std::allocator<abigail::ir::function_decl*> >' @ 0x0x7fffffffc740 }
>>
>>Poking around in gdb found that in compute_diff () the a_base
>>RandomAccessOutputIterator could go back (-1) and then forward (+1)
>>again because there were missing brackets. That is undefined behavior
>>if we are at the beginning of the iterator.
>>
>>Proposed fix attached.
>>
>>Cheers,
>>
>>Mark
>
>>From a5b9bda8a44da06985fbd938b55e209c94893175 Mon Sep 17 00:00:00 2001
>>From: Mark Wielaard <mark@klomp.org>
>>Date: Mon, 11 May 2020 18:55:03 +0200
>>Subject: [PATCH] Don't iterate before the start of a
>>RandomAccessOutputIterator.
>>
>>Found with -D_GLIBCXX_DEBUG. You cannot go before the start of an
>>RandomAccessOutputIterator. Iterator -1 + 1 might seem to work,
>>but is actually undefined behaviour.
>>
>>      * include/abg-diff-utils.h (compute_diff): Put brackets around
>>      p[ux].[xy]() + 1 calculation.
>>
>>Signed-off-by: Mark Wielaard <mark@klomp.org>
>>---
>>include/abg-diff-utils.h | 8 ++++----
>>1 file changed, 4 insertions(+), 4 deletions(-)
>>
>>diff --git a/include/abg-diff-utils.h b/include/abg-diff-utils.h
>>index 3cbdbf33..8bc667d0 100644
>>--- a/include/abg-diff-utils.h
>>+++ b/include/abg-diff-utils.h
>>@@ -1591,8 +1591,8 @@ compute_diff(RandomAccessOutputIterator a_base,
>>      point px, pu;
>>      snake_end_points(snak, px, pu);
>>      compute_diff<RandomAccessOutputIterator,
>>-                EqualityFunctor>(a_base, a_begin, a_base + px.x() + 1,
>>-                                 b_base, b_begin, b_base + px.y() + 1,
>>+                EqualityFunctor>(a_base, a_begin, a_base + (px.x() + 1),
>>+                                 b_base, b_begin, b_base + (px.y() + 1),
>>                                  lcs, tmp_ses0, tmp_ses_len0);
>>
>>      lcs.insert(lcs.end(), trace.begin(), trace.end());
>>@@ -1600,8 +1600,8 @@ compute_diff(RandomAccessOutputIterator a_base,
>>      int tmp_ses_len1 = 0;
>>      edit_script tmp_ses1;
>>      compute_diff<RandomAccessOutputIterator,
>>-                EqualityFunctor>(a_base, a_base + pu.x() + 1, a_end,
>>-                                 b_base, b_base + pu.y() + 1, b_end,
>>+                EqualityFunctor>(a_base, a_base + (pu.x() + 1), a_end,
>>+                                 b_base, b_base + (pu.y() + 1), b_end,
>>                                  lcs, tmp_ses1, tmp_ses_len1);
>>      ABG_ASSERT(tmp_ses0.length() + tmp_ses1.length() == d);
>>      ABG_ASSERT(tmp_ses_len0 + tmp_ses_len1 == d);
>
>That looks good to me. And I can confirm this almost fixing `make check`
>with with _GLIBCXX_DEBUG set. It remains the above reported one.
>
>Reviewed-by: Matthias Maennich <maennich@google.com>
>Tested-by: Matthias Maennich <maennich@google.com>
>
>Cheers,
>Matthias
>
>>--
>>2.18.4
>>
>