Re: [PATCH] configure: add ABIGAIL_DEBUG options

[Mark Wielaard <mark@klomp.org>]

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).

>  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