[PATCH] configure: add ABIGAIL_DEBUG options

[Matthias Maennich]

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
>