Re: Merged "sane" commits from mjw/dwarf_output to dwarf branch

[Mark Wielaard <mjw at redhat dot com>]

On Wed, 2011-03-16 at 14:40 -0700, Roland McGrath wrote:
> > I didn't do that, because I wanted the normal circularity check to catch
> > that. If we get a failure because of any circular reference,
> > self-reference or not, it is probably is something we need to
> > investigate. I cannot imagine when anything like that isn't some bug in
> > the producer.
> 
> Well, that's what dwarflint is for.  There is nothing algorithmically
> difficult about handling direct self-reference in the hash function.
> It exists in some data, whether by producer bug or not, so we have to
> handle it.  The kludge is just too ugly.

We cannot have ugly code :)

I added a dwarflint pass for it.
Attached and on the mjw/check_self_referential_die

Output of warning (on dwarfcmp, the problematic binary):
warning: .debug_info: DIE 0xed86a: attribute containing_type references DIE itself.
warning: .debug_info: DIE 0x1710eb: attribute containing_type references DIE itself.

Petr, if you think that is correct, I like to merge it to the dwarf
branch.

I removed the special DW_AT_containing_type case and just ignore all
self referential attribute values. Although I was tempted to just remove
it completely (since it is wrong, wrong, wrong).

It seems Dodji just posted a patch for gcc that might fix this issue by
properly outputting pointer-to-member-function. I haven't tried it yet
though. http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00949.html

> > It this case it was
> > http://git.fedorahosted.org/git/?p=elfutils.git;a=commit;h=17a29217e9145989ca15479132f8d6d8f80f0258
> 
> Ok, I see the method now.  What is calling that?

It is used to get at the die_info_pair of a debug_info_entry in for
example get_die_info () from value::value_reference to access the
reference_hash of the die that it points to.

Cheers,

Mark

commit fab4929d16ea08b0929795d2cbd44f42cc161a0e
Author: Mark Wielaard <mjw@redhat.com>
Date:   Thu Mar 17 12:46:06 2011 +0100

    Workaround all self referential ref values, not just DW_AT_containing_type.

diff --git a/libdw/c++/dwarf_output b/libdw/c++/dwarf_output
index aeb192c..aab1598 100644
--- a/libdw/c++/dwarf_output
+++ b/libdw/c++/dwarf_output
@@ -1402,10 +1402,11 @@ namespace elfutils
              = dynamic_cast<const entry *> (it->second._m_value);
            if (ref != NULL)
              {
-               // Workaround weird case (bug?)
-               // https://fedorahosted.org/pipermail/elfutils-devel/2011-Februa
-               if (it->first == DW_AT_containing_type
-                   && ref->_m_pending == this)
+               // Workaround weird cases of self-referential DIE.
+               // This really is a bug in the producer and dwarflint
+               // should warn about it. But it is easy to work around
+               // so just do it for now, by ignoring such values.
+               if (ref->_m_pending == this)
                  continue;
                else
                  attr_rhashes ^= ref->get_reference_hash (stack);

commit 2a511cf5310f15e18caeb345ff2a2f8d0f5ca878
Author: Mark Wielaard <mjw@redhat.com>
Date:   Thu Mar 17 12:38:45 2011 +0100

    Add check_self_referential_die pass to dwarflint.

diff --git a/dwarflint/Makefile.am b/dwarflint/Makefile.am
index 7c0a9b2..821bc3b 100644
--- a/dwarflint/Makefile.am
+++ b/dwarflint/Makefile.am
@@ -82,6 +82,7 @@ dwarflint_SOURCES = \
 	check_matching_ranges.cc \
 	check_nodebug.cc \
 	check_range_out_of_scope.cc \
+	check_self_referential_die.cc \
 	locstats.cc \
 	lowlevel_checks.cc lowlevel_checks.hh \
 	\
diff --git a/dwarflint/check_self_referential_die.cc b/dwarflint/check_self_referential_die.cc
new file mode 100644
index 0000000..1f1dfe8
--- /dev/null
+++ b/dwarflint/check_self_referential_die.cc
@@ -0,0 +1,86 @@
+/* Pedantic checking of DWARF files.
+   Copyright (C) 2011 Red Hat, Inc.
+   This file is part of Red Hat elfutils.
+
+   Red Hat elfutils is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by the
+   Free Software Foundation; version 2 of the License.
+
+   Red Hat elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received a copy of the GNU General Public License along
+   with Red Hat elfutils; if not, write to the Free Software Foundation,
+   Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301 USA.
+
+   Red Hat elfutils is an included package of the Open Invention Network.
+   An included package of the Open Invention Network is a package for which
+   Open Invention Network licensees cross-license their patents.  No patent
+   license is granted, either expressly or impliedly, by designation as an
+   included package.  Should you wish to participate in the Open Invention
+   Network licensing program, please visit www.openinventionnetwork.com
+   <http://www.openinventionnetwork.com>.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include "highlevel_check.hh"
+#include "../src/dwarfstrings.h"
+#include "all-dies-it.hh"
+#include "pri.hh"
+#include "messages.hh"
+#include <map>
+
+using elfutils::dwarf;
+
+namespace
+{
+  class check_self_referential_die
+    : public highlevel_check<check_self_referential_die>
+  {
+  public:
+    static checkdescriptor const *descriptor ()
+    {
+      static checkdescriptor cd
+	(checkdescriptor::create ("check_self_referential_die")
+	 .inherit<highlevel_check<check_self_referential_die> > ()
+	 .description (
+"A reference attribute referencing the DIE itself is suspicious.\n"
+"One example is a DW_AT_containing_type pointing to itself.\n"
+" https://fedorahosted.org/pipermail/elfutils-devel/2011-February/001794.html\n";
+		       ));
+      return &cd;
+    }
+
+    explicit check_self_referential_die (checkstack &stack, dwarflint &lint)
+      : highlevel_check<check_self_referential_die> (stack, lint)
+    {
+      for (all_dies_iterator<dwarf> it = all_dies_iterator<dwarf> (dw);
+	   it != all_dies_iterator<dwarf> (); ++it)
+	{
+	  dwarf::debug_info_entry const &die = *it;
+	  for (dwarf::debug_info_entry::attributes_type::const_iterator
+		 at = die.attributes ().begin ();
+	       at != die.attributes ().end (); ++at)
+	    {
+	      dwarf::attr_value const &val = (*at).second;
+	      if (val.what_space () == dwarf::VS_reference)
+		{
+		  dwarf::debug_info_entry ref = *val.reference ();
+		  if (ref.identity () == die.identity ())
+		    wr_message (to_where (die), cat (mc_impact_3,
+						     mc_acc_suboptimal,
+						     mc_die_rel))
+		      << "attribute " << dwarf::attributes::name ((*at).first)
+		      << " references DIE itself." << std::endl;
+		}
+	    }
+	}
+    }
+  };
+
+  reg<check_self_referential_die> reg;
+}