Message ID | 20200511152432.22927-1-maennich@google.com |
---|---|
State | Superseded |
Delegated to: | Dodji Seketeli |
Headers |
From: maennich@google.com (Matthias Maennich) Date: Mon, 11 May 2020 17:24:32 +0200 Subject: [PATCH] configure: add ABIGAIL_DEBUG options Message-ID: <20200511152432.22927-1-maennich@google.com> |
Series |
configure: add ABIGAIL_DEBUG options
|
|
Commit Message
Matthias Männich
May 11, 2020, 3:24 p.m. UTC
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(+)
Comments
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). You also will want to add -D_GLIBCXX_DEBUG which catches various 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Don-t-iterate-before-the-start-of-a-RandomAccessOutp.patch Type: text/x-patch Size: 1843 bytes Desc: not available URL: <https://sourceware.org/pipermail/libabigail/attachments/20200511/6124de7e/attachment.bin>
Don?t do -O0, that skips some of the compiler passes which are needed to generate good quality DWARF. > On May 11, 2020, at 8:24 AM, Matthias Maennich via Libabigail <libabigail@sourceware.org> 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 > + > if test x$ENABLE_ASAN = xyes; then > CFLAGS="$CFLAGS -fsanitize=address" > CXXFLAGS="$CXXFLAGS -fsanitize=address" > -- > 2.26.2.645.ge9eca65c58-goog >
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 >
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.
Hello, Mark Wielaard <mark@klomp.org> a écrit: [...] > 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> Acked-by: Dodji Seketeli <dodji@seketeli.org> Applied to master, thanks!
>> 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,
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
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
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.
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 >> >
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 + if test x$ENABLE_ASAN = xyes; then CFLAGS="$CFLAGS -fsanitize=address" CXXFLAGS="$CXXFLAGS -fsanitize=address"