diff mbox series

configure: add more diagnostic options when ABIGAIL_DEVEL is set

Message ID 20200515092018.31764-1-maennich@google.com
State Committed
Headers
Series configure: add more diagnostic options when ABIGAIL_DEVEL is set |

Commit Message

Matthias Männich May 15, 2020, 9:20 a.m. UTC 
  When exporting ABIGAIL_DEVEL=1, add more flags to ABIGAIL_DEVEL that are
suitable for development to find issues during edit/compile/test time.

To accommodate the need for checked return values of system(), adjust
the tests to explicitely ignore them.

Note, unless bug #25989 is addressed, runtestannotate is failing.
See https://sourceware.org/bugzilla/show_bug.cgi?id=25989 for details.

	* configure.ac: add -D_FORTIFY_SOURCE=2 and -D_GLIBCXX_DEBUG
	compilation defines if ABIGAIL_DEVEL is set

Signed-off-by: Matthias Maennich <maennich@google.com>
---
 configure.ac              | 4 ++--
 tests/test-diff-filter.cc | 2 +-
 tests/test-diff-pkg.cc    | 2 +-
 tests/test-read-write.cc  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

Comments

Dodji Seketeli May 18, 2020, 8:44 a.m. UTC | #1 
Matthias Maennich <maennich@google.com> a écrit:

> When exporting ABIGAIL_DEVEL=1, add more flags to ABIGAIL_DEVEL that are
> suitable for development to find issues during edit/compile/test time.
>
> To accommodate the need for checked return values of system(), adjust
> the tests to explicitely ignore them.
>
> Note, unless bug #25989 is addressed, runtestannotate is failing.
> See https://sourceware.org/bugzilla/show_bug.cgi?id=25989 for details.
>
> 	* configure.ac: add -D_FORTIFY_SOURCE=2 and -D_GLIBCXX_DEBUG
> 	compilation defines if ABIGAIL_DEVEL is set
>
> Signed-off-by: Matthias Maennich <maennich@google.com>
Acked-By: Dodji Seketeli <dodji@redhat.com>

When testing this with gcc 4.8.5, it appeared that -D_FORTIFY_SOURCE
required optimization to work, so I set the optimization level to -O2.

I added some source code changes required by the compiler when building
with the resulting new value of CXXFLAGS.

I have also updated the ChangeLog part of the commit message.

I am adding below the patch that I have applied to master.

Thanks.

Cheers,
Matthias Männich May 18, 2020, 9:01 a.m. UTC | #2 
Hi Dodji!

Thanks for working on this!

On Mon, May 18, 2020 at 10:44:15AM +0200, Dodji Seketeli wrote:
>Matthias Maennich <maennich@google.com> a écrit:
>
>> When exporting ABIGAIL_DEVEL=1, add more flags to ABIGAIL_DEVEL that are
>> suitable for development to find issues during edit/compile/test time.
>>
>> To accommodate the need for checked return values of system(), adjust
>> the tests to explicitely ignore them.
>>
>> Note, unless bug #25989 is addressed, runtestannotate is failing.
>> See https://sourceware.org/bugzilla/show_bug.cgi?id=25989 for details.
>>
>> 	* configure.ac: add -D_FORTIFY_SOURCE=2 and -D_GLIBCXX_DEBUG
>> 	compilation defines if ABIGAIL_DEVEL is set
>>
>> Signed-off-by: Matthias Maennich <maennich@google.com>
>Acked-By: Dodji Seketeli <dodji@redhat.com>
>
>When testing this with gcc 4.8.5, it appeared that -D_FORTIFY_SOURCE
>required optimization to work, so I set the optimization level to -O2.
>
>I added some source code changes required by the compiler when building
>with the resulting new value of CXXFLAGS.
>
>I have also updated the ChangeLog part of the commit message.
>
>I am adding below the patch that I have applied to master.
>
>Thanks.
>
>Cheers,
>

>From d02de5a7846d7f44621c1e100e60f36f5a0c051e Mon Sep 17 00:00:00 2001
>From: Matthias Maennich <maennich@google.com>
>Date: Fri, 15 May 2020 11:20:18 +0200
>Subject: [PATCH] configure: add more diagnostic options when ABIGAIL_DEVEL is
> set
>
>When exporting ABIGAIL_DEVEL=1, add more flags to ABIGAIL_DEVEL that are
>suitable for development to find issues during edit/compile/test time.
>
>The subsequent changes to source and test code are needed to make the
>code compile with ABIGAIL_DEVEL=yes.
>
>Note, unless bug #25989 is addressed, runtestannotate is failing.
>See https://sourceware.org/bugzilla/show_bug.cgi?id=25989 for details.
>
>	* configure.ac: add -D_FORTIFY_SOURCE=2 and -D_GLIBCXX_DEBUG
>	compilation defines if ABIGAIL_DEVEL is set.  Note that with GCC 4.8.5,
>	-D_FORTIFY_SOURCE=2 requires options to be set.  So I am setting
>	the optimization level to -Og.
>	* src/abg-dwarf-reader.cc (read_context::{compute_canonical_die,
>	get_or_compute_canonical_die, associate_die_to_decl,
>	set_canonical_die_offset, schedule_type_for_late_canonicalization,
>	compare_dies}, get_scope_for_die, add_or_update_union_type)
>	(read_debug_info_into_corpus, build_ir_node_from_die): Initialize
>	the 'source' variable.
>	* tests/test-diff-filter.cc (main): Check the return value of the
>	system function.
>	* tests/test-diff-pkg.cc (main): Likewise.
>	* tests/test-read-write.cc (main): Likewise.
>
>Signed-off-by: Matthias Maennich <maennich@google.com>
>Signed-off-by: Dodji Seketeli <dodji@redhat.com>
>---
> configure.ac              |  4 ++--
> src/abg-dwarf-reader.cc   | 22 ++++++++++++----------
> tests/test-diff-filter.cc |  3 ++-
> tests/test-diff-pkg.cc    |  3 ++-
> tests/test-read-write.cc  |  3 ++-
> 5 files changed, 20 insertions(+), 15 deletions(-)
>
>diff --git a/configure.ac b/configure.ac
>index 1d6fe56..36296d6 100644
>--- a/configure.ac
>+++ b/configure.ac
>@@ -617,8 +617,8 @@ DEPS_LIBS="$XML_LIBS $LIBZIP_LIBS $ELF_LIBS $DW_LIBS"
> AC_SUBST(DEPS_LIBS)
> 
> if test x$ABIGAIL_DEVEL != x; then
>-   CFLAGS="-g -Wall -Wextra -Werror"
>-   CXXFLAGS="-g -Wall -Wextra -Werror"
>+   CFLAGS="-Og -Wall -Wextra -Werror -D_FORTIFY_SOURCE=2"
>+   CXXFLAGS="-Og -Wall -Wextra -Werror -D_FORTIFY_SOURCE=2 -D_GLIBCXX_DEBUG"

You mentioned you needed to add -O2, but here it is now -Og. Note that
this is likely unsuitable for ABIGAIL_DEVEL as it slows down `make check`
quite significantly. Hence I put it into ABIGAIL_DEBUG instead.

> fi
> 
> if test x$ABIGAIL_DEBUG != x; then
>diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
>index 5024deb..4bcc652 100644
>--- a/src/abg-dwarf-reader.cc
>+++ b/src/abg-dwarf-reader.cc
>@@ -3060,7 +3060,8 @@ public:
> 			Dwarf_Die &canonical_die,
> 			bool die_as_type) const
>   {
>-    die_source source;
>+    die_source source = NO_DEBUG_INFO_DIE_SOURCE;

Interesting, I did not see those on gcc 9. Maybe the compiler is now
able to look through the initialization that is happening right after.

>+
>     ABG_ASSERT(get_die_source(die, source));
> 
>     Dwarf_Off die_offset = dwarf_dieoffset(const_cast<Dwarf_Die*>(die));
>@@ -3298,7 +3299,7 @@ public:
> 			       size_t where,
> 			       bool die_as_type) const
>   {
>-    die_source source;
>+    die_source source = NO_DEBUG_INFO_DIE_SOURCE;
>     ABG_ASSERT(get_die_source(die, source));
> 
>     offset_offset_map_type &canonical_dies =
>@@ -3521,7 +3522,7 @@ public:
> 			size_t where_offset,
> 			bool do_associate_by_repr = false)
>   {
>-    die_source source;
>+    die_source source = NO_DEBUG_INFO_DIE_SOURCE;
>     ABG_ASSERT(get_die_source(die, source));
> 
>     die_artefact_map_type& m =
>@@ -4151,7 +4152,7 @@ public:
> 			   Dwarf_Off canonical_die_offset,
> 			   bool die_as_type) const
>   {
>-    die_source source;
>+    die_source source = NO_DEBUG_INFO_DIE_SOURCE;
>     ABG_ASSERT(get_die_source(die, source));
> 
>     Dwarf_Off die_offset = dwarf_dieoffset(const_cast<Dwarf_Die*>(die));
>@@ -4732,7 +4733,7 @@ public:
>   schedule_type_for_late_canonicalization(const Dwarf_Die *die)
>   {
>     Dwarf_Off o;
>-    die_source source;
>+    die_source source = NO_DEBUG_INFO_DIE_SOURCE;
> 
>     Dwarf_Die equiv_die;
>     ABG_ASSERT(get_canonical_die(die, equiv_die,
>@@ -11860,7 +11861,8 @@ compare_dies(const read_context& ctxt,
>   Dwarf_Off l_offset = dwarf_dieoffset(const_cast<Dwarf_Die*>(l)),
>     r_offset = dwarf_dieoffset(const_cast<Dwarf_Die*>(r));
>   Dwarf_Off l_canonical_die_offset = 0, r_canonical_die_offset = 0;
>-  die_source l_die_source, r_die_source;
>+  die_source l_die_source = NO_DEBUG_INFO_DIE_SOURCE,
>+    r_die_source = NO_DEBUG_INFO_DIE_SOURCE;
>   ABG_ASSERT(ctxt.get_die_source(l, l_die_source));
>   ABG_ASSERT(ctxt.get_die_source(r, r_die_source));
> 
>@@ -12700,7 +12702,7 @@ get_scope_for_die(read_context& ctxt,
> 		  bool		called_for_public_decl,
> 		  size_t	where_offset)
> {
>-  die_source source_of_die;
>+  die_source source_of_die = NO_DEBUG_INFO_DIE_SOURCE;
>   ABG_ASSERT(ctxt.get_die_source(die, source_of_die));
> 
>   if (is_c_language(ctxt.cur_transl_unit()->get_language()))
>@@ -14141,7 +14143,7 @@ add_or_update_union_type(read_context&	ctxt,
>   if (tag != DW_TAG_union_type)
>     return result;
> 
>-  die_source source;
>+  die_source source = NO_DEBUG_INFO_DIE_SOURCE;
>   ABG_ASSERT(ctxt.get_die_source(die, source));
>   {
>     die_class_or_union_map_type::const_iterator i =
>@@ -16232,7 +16234,7 @@ read_debug_info_into_corpus(read_context& ctxt)
> static void
> maybe_canonicalize_type(const Dwarf_Die *die, read_context& ctxt)
> {
>-  die_source source;
>+  die_source source = NO_DEBUG_INFO_DIE_SOURCE;
>   ABG_ASSERT(ctxt.get_die_source(die, source));
> 
>   size_t die_offset = dwarf_dieoffset(const_cast<Dwarf_Die*>(die));
>@@ -16445,7 +16447,7 @@ build_ir_node_from_die(read_context&	ctxt,
> 	return result;
>     }
> 
>-  die_source source_of_die;
>+  die_source source_of_die = NO_DEBUG_INFO_DIE_SOURCE;
>   ABG_ASSERT(ctxt.get_die_source(die, source_of_die));
> 
>   if ((result = ctxt.lookup_decl_from_die_offset(dwarf_dieoffset(die),
>diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc
>index 000da6d..215485b 100644
>--- a/tests/test-diff-filter.cc
>+++ b/tests/test-diff-filter.cc
>@@ -708,7 +708,8 @@ main()
> 	{
> 	  is_ok = false;
> 	  if (!t->diff_cmd.empty())
>-	    system(t->diff_cmd.c_str());
>+	    if (system(t->diff_cmd.c_str()) == -1)
>+	      cerr << "execution of '" << t->diff_cmd << "' failed\n";

According to the man page I have, 
   "Exit status is 0 if inputs are the same, 1 if different, 2 if trouble."

Hence, should we rather test for != 0 instead?
Same for two more locations of this.

Cheers,
Matthias

> 	  if (!t->error_message.empty())
> 	    cerr << t->error_message << '\n';
> 	}
>diff --git a/tests/test-diff-pkg.cc b/tests/test-diff-pkg.cc
>index 51f117a..8e324da 100644
>--- a/tests/test-diff-pkg.cc
>+++ b/tests/test-diff-pkg.cc
>@@ -840,7 +840,8 @@ main()
> 	{
> 	  is_ok = false;
> 	  if (!t->diff_cmd.empty())
>-	    system(t->diff_cmd.c_str());
>+	    if (system(t->diff_cmd.c_str()) == -1)
>+	      cerr << "execution of '" << t->diff_cmd << "' failed\n";
> 	  if (!t->error_message.empty())
> 	    cerr << t->error_message << '\n';
> 	}
>diff --git a/tests/test-read-write.cc b/tests/test-read-write.cc
>index f32fc25..7d24bee 100644
>--- a/tests/test-read-write.cc
>+++ b/tests/test-read-write.cc
>@@ -404,7 +404,8 @@ main()
> 	    cerr << t->error_message << '\n';
> 
> 	  if (!t->diff_cmd.empty())
>-	    system(t->diff_cmd.c_str());
>+	    if (system(t->diff_cmd.c_str()) == -1)
>+	      cerr << "execution of '" << t->diff_cmd << "' failed\n";
> 	}
>     }
> 
>-- 
>1.8.3.1
>

>
>-- 
>		Dodji
diff mbox series

Patch

diff --git a/configure.ac b/configure.ac
index 1d6fe56bb757..fa4a0a947969 100644
--- a/configure.ac
+++ b/configure.ac
@@ -617,8 +617,8 @@  DEPS_LIBS="$XML_LIBS $LIBZIP_LIBS $ELF_LIBS $DW_LIBS"
 AC_SUBST(DEPS_LIBS)
 
 if test x$ABIGAIL_DEVEL != x; then
-   CFLAGS="-g -Wall -Wextra -Werror"
-   CXXFLAGS="-g -Wall -Wextra -Werror"
+   CFLAGS="-g -Wall -Wextra -Werror -D_FORTIFY_SOURCE=2"
+   CXXFLAGS="-g -Wall -Wextra -Werror -D_FORTIFY_SOURCE=2 -D_GLIBCXX_DEBUG"
 fi
 
 if test x$ABIGAIL_DEBUG != x; then
diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc
index 000da6da188b..60f9c769148a 100644
--- a/tests/test-diff-filter.cc
+++ b/tests/test-diff-filter.cc
@@ -708,7 +708,7 @@  main()
 	{
 	  is_ok = false;
 	  if (!t->diff_cmd.empty())
-	    system(t->diff_cmd.c_str());
+	    (void)system(t->diff_cmd.c_str());
 	  if (!t->error_message.empty())
 	    cerr << t->error_message << '\n';
 	}
diff --git a/tests/test-diff-pkg.cc b/tests/test-diff-pkg.cc
index 51f117a47833..68e095226d92 100644
--- a/tests/test-diff-pkg.cc
+++ b/tests/test-diff-pkg.cc
@@ -840,7 +840,7 @@  main()
 	{
 	  is_ok = false;
 	  if (!t->diff_cmd.empty())
-	    system(t->diff_cmd.c_str());
+	    (void)system(t->diff_cmd.c_str());
 	  if (!t->error_message.empty())
 	    cerr << t->error_message << '\n';
 	}
diff --git a/tests/test-read-write.cc b/tests/test-read-write.cc
index f32fc25d6191..52da379461cf 100644
--- a/tests/test-read-write.cc
+++ b/tests/test-read-write.cc
@@ -404,7 +404,7 @@  main()
 	    cerr << t->error_message << '\n';
 
 	  if (!t->diff_cmd.empty())
-	    system(t->diff_cmd.c_str());
+	    (void)system(t->diff_cmd.c_str());
 	}
     }