* Re: [PATCH] configure: add ABIGAIL_DEBUG options
2020-05-11 20:07 ` Matthias Maennich
@ 2020-05-12 10:47 ` Giuliano Procida
2020-05-12 11:59 ` Mark Wielaard
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Giuliano Procida @ 2020-05-12 10:47 UTC (permalink / raw)
To: Matthias Maennich; +Cc: Mark Wielaard, libabigail, kernel-team, Ben Woodard
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
> >
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] configure: add ABIGAIL_DEBUG options
2020-05-11 20:07 ` Matthias Maennich
2020-05-12 10:47 ` Giuliano Procida
@ 2020-05-12 11:59 ` Mark Wielaard
2020-05-12 14:33 ` Matthias Maennich
2020-05-13 10:43 ` Dodji Seketeli
2020-05-13 11:19 ` Dodji Seketeli
2020-05-27 20:52 ` Matthias Männich
3 siblings, 2 replies; 16+ messages in thread
From: Mark Wielaard @ 2020-05-12 11:59 UTC (permalink / raw)
To: Matthias Maennich; +Cc: libabigail, gprocida, kernel-team, Ben Woodard
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] configure: add ABIGAIL_DEBUG options
2020-05-12 11:59 ` Mark Wielaard
@ 2020-05-12 14:33 ` Matthias Maennich
2020-05-13 10:43 ` Dodji Seketeli
1 sibling, 0 replies; 16+ messages in thread
From: Matthias Maennich @ 2020-05-12 14:33 UTC (permalink / raw)
To: Mark Wielaard; +Cc: libabigail, gprocida, kernel-team, Ben Woodard
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] configure: add ABIGAIL_DEBUG options
2020-05-12 11:59 ` Mark Wielaard
2020-05-12 14:33 ` Matthias Maennich
@ 2020-05-13 10:43 ` Dodji Seketeli
2020-05-13 20:25 ` Matthias Maennich
1 sibling, 1 reply; 16+ messages in thread
From: Dodji Seketeli @ 2020-05-13 10:43 UTC (permalink / raw)
To: Mark Wielaard; +Cc: Matthias Maennich, kernel-team, libabigail, gprocida
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.
--
Dodji
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] configure: add ABIGAIL_DEBUG options
2020-05-13 10:43 ` Dodji Seketeli
@ 2020-05-13 20:25 ` Matthias Maennich
2020-05-14 8:19 ` Dodji Seketeli
0 siblings, 1 reply; 16+ messages in thread
From: Matthias Maennich @ 2020-05-13 20:25 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: Mark Wielaard, kernel-team, libabigail, gprocida
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] configure: add ABIGAIL_DEBUG options
2020-05-13 20:25 ` Matthias Maennich
@ 2020-05-14 8:19 ` Dodji Seketeli
0 siblings, 0 replies; 16+ messages in thread
From: Dodji Seketeli @ 2020-05-14 8:19 UTC (permalink / raw)
To: Matthias Maennich; +Cc: Mark Wielaard, kernel-team, libabigail, gprocida
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.
--
Dodji
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] configure: add ABIGAIL_DEBUG options
2020-05-11 20:07 ` Matthias Maennich
2020-05-12 10:47 ` Giuliano Procida
2020-05-12 11:59 ` Mark Wielaard
@ 2020-05-13 11:19 ` Dodji Seketeli
2020-05-13 20:12 ` Matthias Maennich
2020-05-27 20:52 ` Matthias Männich
3 siblings, 1 reply; 16+ messages in thread
From: Dodji Seketeli @ 2020-05-13 11:19 UTC (permalink / raw)
To: Matthias Maennich; +Cc: Mark Wielaard, kernel-team, libabigail, gprocida
>> 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,
--
Dodji
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] configure: add ABIGAIL_DEBUG options
2020-05-13 11:19 ` Dodji Seketeli
@ 2020-05-13 20:12 ` Matthias Maennich
0 siblings, 0 replies; 16+ messages in thread
From: Matthias Maennich @ 2020-05-13 20:12 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: Mark Wielaard, kernel-team, libabigail, gprocida
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] configure: add ABIGAIL_DEBUG options
2020-05-11 20:07 ` Matthias Maennich
` (2 preceding siblings ...)
2020-05-13 11:19 ` Dodji Seketeli
@ 2020-05-27 20:52 ` Matthias Männich
3 siblings, 0 replies; 16+ messages in thread
From: Matthias Männich @ 2020-05-27 20:52 UTC (permalink / raw)
To: Mark Wielaard
Cc: libabigail, Giuliano Procida, Android Kernel Team, Ben Woodard
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
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread