Re: [PATCH] configure: add ABIGAIL_DEBUG options

[Matthias Maennich <maennich@google.com>]

On Tue, May 12, 2020 at 01:59:19PM +0200, Mark Wielaard wrote:
>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).
>
>> > 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).
>

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.

Let's give more people a chance to share their opinion before I send
another version of this.

Cheers,
Matthias

>>  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.
>
>Yes, I see that as well with GCC 10 (was previously using GCC 4.8,
>which only showed that other issue).
>
>It looks like Giuliano's suggestion fixes it. But it also subtly
>changes the sorting order (reference-type-def now comes before
>qualified-type-def), causing some make check failures (runtestannotate
>and runtestreadwrite). But that might be the point. I don't really
>understand the type_topo_comp Compare operator. But it is clear that
>the part that compares the qualifiers which Giuliano identified can
>cause the irreflexive requirement to break.
>
>> > 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>
>
>Thanks,
>
>Mark