[PATCH] configure: add ABIGAIL_DEBUG options
Giuliano Procida
gprocida@google.com
Tue May 12 10:47:02 GMT 2020
Looks like this code:
if (s1 == s2)
if (qualified_type_def * q = is_qualified_type(f))
if (q->get_cv_quals() == qualified_type_def::CV_NONE)
return true;
return (s1 < s2);
I'm not sure what it's trying to achieve. I'm assuming that s1 and s2
incorporate any outer CV qualifiers in the pretty representation.
Perhaps it should just be:
if (s1 != s2)
return (s1 < s2);
Giuliano.
On Mon, 11 May 2020 at 21:07, Matthias Maennich <maennich@google.com> 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.
>
> >
> >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
> >
>
>
More information about the Libabigail
mailing list