This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix the ODR checker again


This time, the patch compares the set of lines attached to the first
instruction, rather than just the exact line it wound up coming from.
This should include the actual line number of the function definition
(given gcc's debug info output), fixing the ODR checker for mixes of
optimized and unoptimized versions of the same function.

`cvs add` wanted write access to the server, so the two added files
are attached instead of inside the patch.

I tested this with `make -j8 all-gold && make -j8 check-gold` on
x86_64-unknown-linux-gnu.

gold/ChangeLog:
2011-03-03  Jeffrey Yasskin  <jyasskin@google.com>

	* dwarf_reader.cc (Sized_dwarf_line_info): Include all lines, but
mark earlier ones as non-canonical
	(offset_to_iterator): Update search target and example
	(do_addr2line): Return extra lines in a vector*
	(format_file_lineno): Extract from do_addr2line
	(one_addr2line): Add vector* out-param
	* dwarf_reader.h (Offset_to_lineno_entry): New field recording when a
lineno entry appeared last for its instruction
	(Dwarf_line_info): Add vector* out-param
	* object.cc (Relocate_info): Pass NULL for the vector* out-param
	* symtab.cc (Odr_violation_compare): Include the lineno in the
comparison again.
	(linenos_from_loc): New. Combine the canonical line for an address
with its other lines.
	(True_if_intersect): New. Helper functor to make std::set_intersection a query.
	(detect_odr_violations): Compare sets of lines instead of just one
line for each function. This became less deterministic, but has fewer
false positives.
	* symtab.h: Declarations.
	* testsuite/Makefile.am (odr_violation2.o): Compile with -O2 to mix
an optimized and non-optimized object in the same binary
	(odr_violation2.so): Same.
	* testsuite/Makefile.in: Regenerate from Makefile.am.
	* testsuite/debug_msg.cc (main):
	* testsuite/debug_msg.sh: Update line numbers and add assertions.
	* testsuite/odr_violation1.cc: Use OdrDerived, in a non-optimized context.
	* testsuite/odr_violation2.cc: Make sure Ordering::operator() isn't
inlined, and use OdrDerived in an optimized context.
	* testsuite/odr_header1.h: Defines OdrDerived, where optimization
will change the first-instruction-in-the-destructor's file and line
number.
	* testsuite/odr_header2.h: Defines OdrBase.
#include "odr_header2.h"

struct OdrDerived : OdrBase {
  ~OdrDerived() {
  }
};
struct OdrBase {
  virtual ~OdrBase() {
  }
};
? gold/testsuite/odr_header1.h
? gold/testsuite/odr_header2.h
Index: gold/dwarf_reader.cc
===================================================================
RCS file: /cvs/src/src/gold/dwarf_reader.cc,v
retrieving revision 1.31
diff -u -u -p -r1.31 dwarf_reader.cc
--- gold/dwarf_reader.cc	20 Dec 2010 18:37:36 -0000	1.31
+++ gold/dwarf_reader.cc	8 Mar 2011 00:32:25 -0000
@@ -492,19 +492,18 @@ Sized_dwarf_line_info<size, big_endian>:
             {
               Offset_to_lineno_entry entry
                   = { lsm.address, this->current_header_index_,
-                      lsm.file_num, lsm.line_num };
+                      lsm.file_num, lsm.line_num, true };
 	      std::vector<Offset_to_lineno_entry>&
 		map(this->line_number_map_[lsm.shndx]);
 	      // If we see two consecutive entries with the same
-	      // offset and a real line number, then always use the
-	      // second one.
+	      // offset and a real line number, then mark the first
+	      // one as non-canonical.
 	      if (!map.empty()
 		  && (map.back().offset == static_cast<off_t>(lsm.address))
 		  && lsm.line_num != -1
 		  && map.back().line_num != -1)
-		map.back() = entry;
-	      else
-		map.push_back(entry);
+		map.back().last_line_for_offset = false;
+	      map.push_back(entry);
             }
           lineptr += oplength;
         }
@@ -612,7 +611,7 @@ static std::vector<Offset_to_lineno_entr
 offset_to_iterator(const std::vector<Offset_to_lineno_entry>* offsets,
                    off_t offset)
 {
-  const Offset_to_lineno_entry lookup_key = { offset, 0, 0, 0 };
+  const Offset_to_lineno_entry lookup_key = { offset, 0, 0, 0, true };
 
   // lower_bound() returns the smallest offset which is >= lookup_key.
   // If no offset in offsets is >= lookup_key, returns end().
@@ -621,25 +620,27 @@ offset_to_iterator(const std::vector<Off
 
   // This code is easiest to understand with a concrete example.
   // Here's a possible offsets array:
-  // {{offset = 3211, header_num = 0, file_num = 1, line_num = 16},  // 0
-  //  {offset = 3224, header_num = 0, file_num = 1, line_num = 20},  // 1
-  //  {offset = 3226, header_num = 0, file_num = 1, line_num = 22},  // 2
-  //  {offset = 3231, header_num = 0, file_num = 1, line_num = 25},  // 3
-  //  {offset = 3232, header_num = 0, file_num = 1, line_num = -1},  // 4
-  //  {offset = 3232, header_num = 0, file_num = 1, line_num = 65},  // 5
-  //  {offset = 3235, header_num = 0, file_num = 1, line_num = 66},  // 6
-  //  {offset = 3236, header_num = 0, file_num = 1, line_num = -1},  // 7
-  //  {offset = 5764, header_num = 0, file_num = 1, line_num = 47},  // 8
-  //  {offset = 5765, header_num = 0, file_num = 1, line_num = 48},  // 9
-  //  {offset = 5767, header_num = 0, file_num = 1, line_num = 49},  // 10
-  //  {offset = 5768, header_num = 0, file_num = 1, line_num = 50},  // 11
-  //  {offset = 5773, header_num = 0, file_num = 1, line_num = -1},  // 12
-  //  {offset = 5787, header_num = 1, file_num = 1, line_num = 19},  // 13
-  //  {offset = 5790, header_num = 1, file_num = 1, line_num = 20},  // 14
-  //  {offset = 5793, header_num = 1, file_num = 1, line_num = 67},  // 15
-  //  {offset = 5793, header_num = 1, file_num = 1, line_num = -1},  // 16
-  //  {offset = 5795, header_num = 1, file_num = 1, line_num = 68},  // 17
-  //  {offset = 5798, header_num = 1, file_num = 1, line_num = -1},  // 18
+  // {{offset = 3211, header_num = 0, file_num = 1, line_num = 16, last},  // 0
+  //  {offset = 3224, header_num = 0, file_num = 1, line_num = 20, last},  // 1
+  //  {offset = 3226, header_num = 0, file_num = 1, line_num = 22, last},  // 2
+  //  {offset = 3231, header_num = 0, file_num = 1, line_num = 25, last},  // 3
+  //  {offset = 3232, header_num = 0, file_num = 1, line_num = -1, last},  // 4
+  //  {offset = 3232, header_num = 0, file_num = 1, line_num = 65, last},  // 5
+  //  {offset = 3235, header_num = 0, file_num = 1, line_num = 66, last},  // 6
+  //  {offset = 3236, header_num = 0, file_num = 1, line_num = -1, last},  // 7
+  //  {offset = 5764, header_num = 0, file_num = 1, line_num = 48, last},  // 8
+  //  {offset = 5764, header_num = 0, file_num = 1, line_num = 47, !last}, // 9
+  //  {offset = 5765, header_num = 0, file_num = 1, line_num = 49, last},  // 10
+  //  {offset = 5767, header_num = 0, file_num = 1, line_num = 50, last},  // 11
+  //  {offset = 5768, header_num = 0, file_num = 1, line_num = 51, last},  // 12
+  //  {offset = 5773, header_num = 0, file_num = 1, line_num = -1, last},  // 13
+  //  {offset = 5787, header_num = 1, file_num = 1, line_num = 19, last},  // 14
+  //  {offset = 5790, header_num = 1, file_num = 1, line_num = 20, last},  // 15
+  //  {offset = 5793, header_num = 1, file_num = 1, line_num = 67, last},  // 16
+  //  {offset = 5793, header_num = 1, file_num = 1, line_num = -1, last},  // 17
+  //  {offset = 5793, header_num = 1, file_num = 1, line_num = 66, !last}, // 18
+  //  {offset = 5795, header_num = 1, file_num = 1, line_num = 68, last},  // 19
+  //  {offset = 5798, header_num = 1, file_num = 1, line_num = -1, last},  // 20
   // The entries with line_num == -1 mark the end of a function: the
   // associated offset is one past the last instruction in the
   // function.  This can correspond to the beginning of the next
@@ -651,7 +652,7 @@ offset_to_iterator(const std::vector<Off
   //         offsets[0].  Since it's not an exact match and we're
   //         at the beginning of offsets, we return end() (invalid).
   // Case 2: lookup_key has offset 10000.  lower_bound returns
-  //         offset[19] (end()).  We return end() (invalid).
+  //         offset[21] (end()).  We return end() (invalid).
   // Case 3: lookup_key has offset == 3211.  lower_bound matches
   //         offsets[0] exactly, and that's the entry we return.
   // Case 4: lookup_key has offset == 3232.  lower_bound returns
@@ -670,16 +671,17 @@ offset_to_iterator(const std::vector<Off
   //         end-of-function, we know lookup_key is between
   //         functions, so we return end() (not a valid offset).
   // Case 7: lookup_key has offset == 5794.  lower_bound returns
-  //         offsets[17].  Since it's not an exact match, we back
-  //         up to offsets[15].  Note we back up to the *first*
-  //         entry with offset 5793, not just offsets[17-1].
-  //         We note offsets[15] is a valid entry, so we return it.
-  //         If offsets[15] had had line_num == -1, we would have
-  //         checked offsets[16].  The reason for this is that
-  //         15 and 16 can be in an arbitrary order, since we sort
-  //         only by offset.  (Note it doesn't help to use line_number
-  //         as a secondary sort key, since sometimes we want the -1
-  //         to be first and sometimes we want it to be last.)
+  //         offsets[19].  Since it's not an exact match, we back
+  //         up to offsets[16].  Note we back up to the *first*
+  //         entry with offset 5793, not just offsets[19-1].
+  //         We note offsets[16] is a valid entry, so we return it.
+  //         If offsets[16] had had line_num == -1, we would have
+  //         checked offsets[17].  The reason for this is that
+  //         16 and 17 can be in an arbitrary order, since we sort
+  //         only by offset and last_line_for_offset.  (Note it
+  //         doesn't help to use line_number as a tertiary sort key,
+  //         since sometimes we want the -1 to be first and sometimes
+  //         we want it to be last.)
 
   // This deals with cases (1) and (2).
   if ((it == offsets->begin() && offset < it->offset)
@@ -710,19 +712,21 @@ offset_to_iterator(const std::vector<Off
 
   // This handles cases (5), (6), and (7): if any entry in the
   // equal_range [it, range_end) has a line_num != -1, it's a valid
-  // match.  If not, we're not in a function.
+  // match.  If not, we're not in a function.  The line number we saw
+  // last for an offset will be sorted first, so it'll get returned if
+  // it's present.
   for (; it != range_end; ++it)
     if (it->line_num != -1)
       return it;
   return offsets->end();
 }
 
-// Return a string for a file name and line number.
-
 template<int size, bool big_endian>
 std::string
-Sized_dwarf_line_info<size, big_endian>::do_addr2line(unsigned int shndx,
-                                                      off_t offset)
+Sized_dwarf_line_info<size, big_endian>::do_addr2line(
+    unsigned int shndx,
+    off_t offset,
+    std::vector<std::string>* other_lines)
 {
   if (this->data_valid_ == false)
     return "";
@@ -743,21 +747,37 @@ Sized_dwarf_line_info<size, big_endian>:
   if (it == offsets->end())
     return "";
 
-  // Convert the file_num + line_num into a string.
+  std::string result = format_file_lineno(*it);
+  if (other_lines != NULL)
+    for (++it; it != offsets->end() && it->offset == offset; ++it)
+      {
+        if (it->line_num == -1)
+          continue;  // The end of a previous function.
+        other_lines->push_back(format_file_lineno(*it));
+      }
+  return result;
+}
+
+// Convert the file_num + line_num into a string.
+template<int size, bool big_endian>
+std::string
+Sized_dwarf_line_info<size, big_endian>::format_file_lineno(
+    const Offset_to_lineno_entry& loc) const
+{
   std::string ret;
 
-  gold_assert(it->header_num < static_cast<int>(this->files_.size()));
-  gold_assert(it->file_num
-	      < static_cast<int>(this->files_[it->header_num].size()));
+  gold_assert(loc.header_num < static_cast<int>(this->files_.size()));
+  gold_assert(loc.file_num
+	      < static_cast<int>(this->files_[loc.header_num].size()));
   const std::pair<int, std::string>& filename_pair
-      = this->files_[it->header_num][it->file_num];
+      = this->files_[loc.header_num][loc.file_num];
   const std::string& filename = filename_pair.second;
 
-  gold_assert(it->header_num < static_cast<int>(this->directories_.size()));
+  gold_assert(loc.header_num < static_cast<int>(this->directories_.size()));
   gold_assert(filename_pair.first
-              < static_cast<int>(this->directories_[it->header_num].size()));
+              < static_cast<int>(this->directories_[loc.header_num].size()));
   const std::string& dirname
-      = this->directories_[it->header_num][filename_pair.first];
+      = this->directories_[loc.header_num][filename_pair.first];
 
   if (!dirname.empty())
     {
@@ -769,7 +789,7 @@ Sized_dwarf_line_info<size, big_endian>:
     ret = "(unknown)";
 
   char buffer[64];   // enough to hold a line number
-  snprintf(buffer, sizeof(buffer), "%d", it->line_num);
+  snprintf(buffer, sizeof(buffer), "%d", loc.line_num);
   ret += ":";
   ret += buffer;
 
@@ -803,7 +823,8 @@ static std::vector<Addr2line_cache_entry
 std::string
 Dwarf_line_info::one_addr2line(Object* object,
                                unsigned int shndx, off_t offset,
-                               size_t cache_size)
+                               size_t cache_size,
+                               std::vector<std::string>* other_lines)
 {
   Dwarf_line_info* lineinfo = NULL;
   std::vector<Addr2line_cache_entry>::iterator it;
@@ -854,7 +875,7 @@ Dwarf_line_info::one_addr2line(Object* o
   }
 
   // Now that we have our object, figure out the answer
-  std::string retval = lineinfo->addr2line(shndx, offset);
+  std::string retval = lineinfo->addr2line(shndx, offset, other_lines);
 
   // Finally, if our cache has grown too big, delete old objects.  We
   // assume the common (probably only) case is deleting only one object.
Index: gold/dwarf_reader.h
===================================================================
RCS file: /cvs/src/src/gold/dwarf_reader.h,v
retrieving revision 1.19
diff -u -u -p -r1.19 dwarf_reader.h
--- gold/dwarf_reader.h	20 Dec 2010 18:37:36 -0000	1.19
+++ gold/dwarf_reader.h	8 Mar 2011 00:32:25 -0000
@@ -25,6 +25,7 @@
 
 #include <vector>
 #include <map>
+#include <limits.h>
 
 #include "elfcpp.h"
 #include "elfcpp_swap.h"
@@ -45,13 +46,23 @@ struct Offset_to_lineno_entry
   off_t offset;
   int header_num;  // which file-list to use (i.e. which .o file are we in)
   int file_num;    // a pointer into files_
-  int line_num;    // the line number in the source file
-
-  // When we add entries to the table, we always use the last entry
-  // with a given offset.  Given proper DWARF info, this should ensure
-  // that the offset is a sufficient sort key.
+  // the line number in the source file.  -1 to indicate end-of-function.
+  int line_num : sizeof(int) * CHAR_BIT - 1;
+  // true if this was the last entry for the current offset, meaning
+  // it's the line that actually applies.
+  unsigned int last_line_for_offset : 1;
+
+  // This sorts by offsets first, and then puts the correct line to
+  // report for a given offset at the beginning of the run of equal
+  // offsets (so that asking for 1 line gives the best answer).  This
+  // is not a total ordering.
   bool operator<(const Offset_to_lineno_entry& that) const
-  { return this->offset < that.offset; }
+  {
+    if (this->offset != that.offset)
+      return this->offset < that.offset;
+    // Note the '>' which makes this sort 'true' first.
+    return this->last_line_for_offset > that.last_line_for_offset;
+  }
 };
 
 // This class is used to read the line information from the debugging
@@ -70,10 +81,13 @@ class Dwarf_line_info
   // Given a section number and an offset, returns the associated
   // file and line-number, as a string: "file:lineno".  If unable
   // to do the mapping, returns the empty string.  You must call
-  // read_line_mappings() before calling this function.
+  // read_line_mappings() before calling this function.  If
+  // 'other_lines' is non-NULL, fills that in with other line
+  // numbers assigned to the same offset.
   std::string
-  addr2line(unsigned int shndx, off_t offset)
-  { return do_addr2line(shndx, offset); }
+  addr2line(unsigned int shndx, off_t offset,
+            std::vector<std::string>* other_lines)
+  { return do_addr2line(shndx, offset, other_lines); }
 
   // A helper function for a single addr2line lookup.  It also keeps a
   // cache of the last CACHE_SIZE Dwarf_line_info objects it created;
@@ -83,7 +97,7 @@ class Dwarf_line_info
   // NOTE: Not thread-safe, so only call from one thread at a time.
   static std::string
   one_addr2line(Object* object, unsigned int shndx, off_t offset,
-                size_t cache_size);
+                size_t cache_size, std::vector<std::string>* other_lines);
 
   // This reclaims all the memory that one_addr2line may have cached.
   // Use this when you know you will not be calling one_addr2line again.
@@ -92,7 +106,8 @@ class Dwarf_line_info
 
  private:
   virtual std::string
-  do_addr2line(unsigned int shndx, off_t offset) = 0;
+  do_addr2line(unsigned int shndx, off_t offset,
+               std::vector<std::string>* other_lines) = 0;
 };
 
 template<int size, bool big_endian>
@@ -106,7 +121,12 @@ class Sized_dwarf_line_info : public Dwa
 
  private:
   std::string
-  do_addr2line(unsigned int shndx, off_t offset);
+  do_addr2line(unsigned int shndx, off_t offset,
+               std::vector<std::string>* other_lines);
+
+  // Formats a file and line number to a string like "dirname/filename:lineno".
+  std::string
+  format_file_lineno(const Offset_to_lineno_entry& lineno) const;
 
   // Start processing line info, and populates the offset_map_.
   // If SHNDX is non-negative, only store debug information that
Index: gold/object.cc
===================================================================
RCS file: /cvs/src/src/gold/object.cc,v
retrieving revision 1.134
diff -u -u -p -r1.134 object.cc
--- gold/object.cc	14 Dec 2010 19:03:30 -0000	1.134
+++ gold/object.cc	8 Mar 2011 00:32:25 -0000
@@ -2582,7 +2582,7 @@ Relocate_info<size, big_endian>::locatio
 
   Sized_dwarf_line_info<size, big_endian> line_info(this->object);
   // This will be "" if we failed to parse the debug info for any reason.
-  file_and_lineno = line_info.addr2line(this->data_shndx, offset);
+  file_and_lineno = line_info.addr2line(this->data_shndx, offset, NULL);
 
   std::string ret(this->object->name());
   ret += ':';
Index: gold/symtab.cc
===================================================================
RCS file: /cvs/src/src/gold/symtab.cc,v
retrieving revision 1.148
diff -u -u -p -r1.148 symtab.cc
--- gold/symtab.cc	18 Feb 2011 14:55:33 -0000	1.148
+++ gold/symtab.cc	8 Mar 2011 00:32:25 -0000
@@ -3025,7 +3025,7 @@ Symbol_table::print_stats() const
 
 // We check for ODR violations by looking for symbols with the same
 // name for which the debugging information reports that they were
-// defined in different source locations.  When comparing the source
+// defined in disjoint source locations.  When comparing the source
 // location, we consider instances with the same base filename to be
 // the same.  This is because different object files/shared libraries
 // can include the same header file using different paths, and
@@ -3035,7 +3035,7 @@ Symbol_table::print_stats() const
 
 // This struct is used to compare line information, as returned by
 // Dwarf_line_info::one_addr2line.  It implements a < comparison
-// operator used with std::set.
+// operator used with std::sort.
 
 struct Odr_violation_compare
 {
@@ -3043,32 +3043,75 @@ struct Odr_violation_compare
   operator()(const std::string& s1, const std::string& s2) const
   {
     // Inputs should be of the form "dirname/filename:linenum" where
-    // "dirname/" is optional.  We want to compare just the filename.
+    // "dirname/" is optional.  We want to compare just the filename:linenum.
 
-    // Find the last '/' and ':' in each string.
+    // Find the last '/' in each string.
     std::string::size_type s1begin = s1.rfind('/');
     std::string::size_type s2begin = s2.rfind('/');
-    std::string::size_type s1end = s1.rfind(':');
-    std::string::size_type s2end = s2.rfind(':');
     // If there was no '/' in a string, start at the beginning.
     if (s1begin == std::string::npos)
       s1begin = 0;
     if (s2begin == std::string::npos)
       s2begin = 0;
-    // If the ':' appeared in the directory name, compare to the end
-    // of the string.
-    if (s1end < s1begin)
-      s1end = s1.size();
-    if (s2end < s2begin)
-      s2end = s2.size();
-    // Compare takes lengths, not end indices.
-    return s1.compare(s1begin, s1end - s1begin,
-		      s2, s2begin, s2end - s2begin) < 0;
+    return s1.compare(s1begin, std::string::npos,
+		      s2, s2begin, std::string::npos) < 0;
   }
 };
 
+std::vector<std::string>
+Symbol_table::linenos_from_loc(const Task* task,
+                               const Symbol_location& loc) const
+{
+  // We need to lock the object in order to read it.  This
+  // means that we have to run in a singleton Task.  If we
+  // want to run this in a general Task for better
+  // performance, we will need one Task for object, plus
+  // appropriate locking to ensure that we don't conflict with
+  // other uses of the object.  Also note, one_addr2line is not
+  // currently thread-safe.
+  Task_lock_obj<Object> tl(task, loc.object);
+
+  std::vector<std::string> result;
+  // 16 is the size of the object-cache that one_addr2line should use.
+  std::string canonical_result = Dwarf_line_info::one_addr2line(
+      loc.object, loc.shndx, loc.offset, 16, &result);
+  if (!canonical_result.empty())
+    result.push_back(canonical_result);
+  return result;
+}
+
+// OutputIterator whose operator bool() becomes true when it's
+// assigned to.  This allows it to be used with
+// std::set_intersection() to check for intersection rather than
+// computing the intersection.
+struct True_if_intersect
+{
+  True_if_intersect() : value_(false)
+  {}
+
+  operator bool()
+  { return value_; }
+
+  True_if_intersect& operator++()
+  { return *this; }
+
+  True_if_intersect& operator*()
+  { return *this; }
+
+  template<typename T>
+  True_if_intersect& operator=(const T&)
+  {
+    value_ = true;
+    return *this;
+  }
+
+ private:
+  bool value_;
+};
+
 // Check candidate_odr_violations_ to find symbols with the same name
-// but apparently different definitions (different source-file/line-no).
+// but apparently different definitions (different source-file/line-no
+// for each line assigned to the first instruction).
 
 void
 Symbol_table::detect_odr_violations(const Task* task,
@@ -3078,48 +3121,68 @@ Symbol_table::detect_odr_violations(cons
        it != candidate_odr_violations_.end();
        ++it)
     {
-      const char* symbol_name = it->first;
-      // Maps from symbol location to a sample object file we found
-      // that location in.  We use a sorted map so the location order
-      // is deterministic, but we only store an arbitrary object file
-      // to avoid copying lots of names.
-      std::map<std::string, std::string, Odr_violation_compare> line_nums;
-
-      for (Unordered_set<Symbol_location, Symbol_location_hash>::const_iterator
-               locs = it->second.begin();
-           locs != it->second.end();
-           ++locs)
+      const char* const symbol_name = it->first;
+
+      std::string first_object_name;
+      std::vector<std::string> first_object_linenos;
+
+      Unordered_set<Symbol_location, Symbol_location_hash>::const_iterator
+          locs = it->second.begin(), locs_end = it->second.end();
+      for (; locs != locs_end && first_object_linenos.empty(); ++locs)
         {
-	  // We need to lock the object in order to read it.  This
-	  // means that we have to run in a singleton Task.  If we
-	  // want to run this in a general Task for better
-	  // performance, we will need one Task for object, plus
-	  // appropriate locking to ensure that we don't conflict with
-	  // other uses of the object.  Also note, one_addr2line is not
-          // currently thread-safe.
-	  Task_lock_obj<Object> tl(task, locs->object);
-          // 16 is the size of the object-cache that one_addr2line should use.
-          std::string lineno = Dwarf_line_info::one_addr2line(
-              locs->object, locs->shndx, locs->offset, 16);
-          if (!lineno.empty())
-            {
-              std::string& sample_object = line_nums[lineno];
-              if (sample_object.empty())
-                sample_object = locs->object->name();
-            }
+          // Save the line numbers from the first definition to
+          // compare to the other definitions.  Ideally, we'd compare
+          // every definition to every other, but we don't want to
+          // take O(N^2) time to do this.  This shortcut may cause
+          // false negatives that appear or disappear depending on the
+          // link order, but it won't cause false positives.
+          first_object_name = locs->object->name();
+          first_object_linenos = linenos_from_loc(task, *locs);
         }
 
-      if (line_nums.size() > 1)
+      // Sort by Odr_violation_compare to make std::set_intersection work.
+      std::sort(first_object_linenos.begin(), first_object_linenos.end(),
+                Odr_violation_compare());
+
+      for (; locs != locs_end; ++locs)
         {
-          gold_warning(_("while linking %s: symbol '%s' defined in multiple "
-                         "places (possible ODR violation):"),
-                       output_file_name, demangle(symbol_name).c_str());
-          for (std::map<std::string, std::string>::const_iterator it2 =
-		 line_nums.begin();
-	       it2 != line_nums.end();
-	       ++it2)
-            fprintf(stderr, _("  %s from %s\n"),
-                    it2->first.c_str(), it2->second.c_str());
+          std::vector<std::string> linenos = linenos_from_loc(task, *locs);
+          // linenos will be empty if we couldn't parse the debug info.
+          if (linenos.empty())
+            continue;
+          // Sort by Odr_violation_compare to make std::set_intersection work.
+          std::sort(linenos.begin(), linenos.end(), Odr_violation_compare());
+
+          if (!std::set_intersection(first_object_linenos.begin(),
+                                     first_object_linenos.end(),
+                                     linenos.begin(),
+                                     linenos.end(),
+                                     True_if_intersect(),
+                                     Odr_violation_compare()))
+            {
+              gold_warning(_("while linking %s: symbol '%s' defined in "
+                             "multiple places (possible ODR violation):"),
+                           output_file_name, demangle(symbol_name).c_str());
+              // This only prints one location from each definition,
+              // which may not be the location we expect to intersect
+              // with another definition.  We could print the whole
+              // set of locations, but that seems too verbose.
+              gold_assert(!first_object_linenos.empty());
+              gold_assert(!linenos.empty());
+              fprintf(stderr, _("  %s from %s\n"),
+                      first_object_linenos[0].c_str(),
+                      first_object_name.c_str());
+              fprintf(stderr, _("  %s from %s\n"),
+                      linenos[0].c_str(),
+                      locs->object->name().c_str());
+              // Only print one broken pair, to avoid needing to
+              // compare against a list of the disjoint definition
+              // locations we've found so far.  (If we kept comparing
+              // against just the first one, we'd get a lot of
+              // redundant complaints about the second definition
+              // location.)
+              break;
+            }
         }
     }
   // We only call one_addr2line() in this function, so we can clear its cache.
Index: gold/symtab.h
===================================================================
RCS file: /cvs/src/src/gold/symtab.h,v
retrieving revision 1.117
diff -u -u -p -r1.117 symtab.h
--- gold/symtab.h	14 Dec 2010 19:03:30 -0000	1.117
+++ gold/symtab.h	8 Mar 2011 00:32:25 -0000
@@ -1560,6 +1560,8 @@ class Symbol_table
   typedef Unordered_map<Symbol_table_key, Symbol*, Symbol_table_hash,
 			Symbol_table_eq> Symbol_table_type;
 
+  struct Symbol_location;  // See definition below.
+
   // Make FROM a forwarder symbol to TO.
   void
   make_forwarder(Symbol* from, Symbol* to);
@@ -1707,6 +1709,12 @@ class Symbol_table
   do_allocate_commons_list(Layout*, Commons_section_type, Commons_type*,
 			   Mapfile*, Sort_commons_order);
 
+  // Returns all of the lines attached to loc, not just the one the
+  // instruction actually came from.  This helps the ODR checker avoid
+  // false positives.
+  std::vector<std::string>
+  linenos_from_loc(const Task* task, const Symbol_location& loc) const;
+
   // Implement detect_odr_violations.
   template<int size, bool big_endian>
   void
Index: gold/testsuite/Makefile.am
===================================================================
RCS file: /cvs/src/src/gold/testsuite/Makefile.am,v
retrieving revision 1.154
diff -u -u -p -r1.154 Makefile.am
--- gold/testsuite/Makefile.am	16 Dec 2010 18:28:43 -0000	1.154
+++ gold/testsuite/Makefile.am	8 Mar 2011 00:32:25 -0000
@@ -849,8 +849,10 @@ debug_msg.o: debug_msg.cc
 	$(CXXCOMPILE) -O0 -g -c -w -o $@ $(srcdir)/debug_msg.cc
 odr_violation1.o: odr_violation1.cc
 	$(CXXCOMPILE) -O0 -g -c -w -o $@ $(srcdir)/odr_violation1.cc
+# Compile with different optimization flags to check that rearranged
+# instructions don't cause a false positive.
 odr_violation2.o: odr_violation2.cc
-	$(CXXCOMPILE) -O0 -g -c -w -o $@ $(srcdir)/odr_violation2.cc
+	$(CXXCOMPILE) -O2 -g -c -w -o $@ $(srcdir)/odr_violation2.cc
 debug_msg.err: debug_msg.o odr_violation1.o odr_violation2.o gcctestdir/ld
 	@echo $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg debug_msg.o odr_violation1.o odr_violation2.o "2>$@"
 	@if $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg debug_msg.o odr_violation1.o odr_violation2.o 2>$@; \
@@ -868,7 +870,7 @@ debug_msg.so: debug_msg.cc gcctestdir/ld
 odr_violation1.so: odr_violation1.cc gcctestdir/ld
 	$(CXXCOMPILE) -Bgcctestdir/ -O0 -g -shared -fPIC -w -o $@ $(srcdir)/odr_violation1.cc
 odr_violation2.so: odr_violation2.cc gcctestdir/ld
-	$(CXXCOMPILE) -Bgcctestdir/ -O0 -g -shared -fPIC -w -o $@ $(srcdir)/odr_violation2.cc
+	$(CXXCOMPILE) -Bgcctestdir/ -O2 -g -shared -fPIC -w -o $@ $(srcdir)/odr_violation2.cc
 debug_msg_so.err: debug_msg.so odr_violation1.so odr_violation2.so gcctestdir/ld
 	@echo $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg_so debug_msg.so odr_violation1.so odr_violation2.so "2>$@"
 	@if $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg_so debug_msg.so odr_violation1.so odr_violation2.so 2>$@; \
@@ -887,7 +889,7 @@ debug_msg_ndebug.so: debug_msg.cc gcctes
 odr_violation1_ndebug.so: odr_violation1.cc gcctestdir/ld
 	$(CXXCOMPILE) -Bgcctestdir/ -O0 -g0 -shared -fPIC -w -o $@ $(srcdir)/odr_violation1.cc
 odr_violation2_ndebug.so: odr_violation2.cc gcctestdir/ld
-	$(CXXCOMPILE) -Bgcctestdir/ -O0 -g0 -shared -fPIC -w -o $@ $(srcdir)/odr_violation2.cc
+	$(CXXCOMPILE) -Bgcctestdir/ -O2 -g0 -shared -fPIC -w -o $@ $(srcdir)/odr_violation2.cc
 debug_msg_ndebug.err: debug_msg_ndebug.so odr_violation1_ndebug.so odr_violation2_ndebug.so gcctestdir/ld
 	@echo $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg_ndebug debug_msg_ndebug.so odr_violation1_ndebug.so odr_violation2_ndebug.so "2>$@"
 	@if $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg_ndebug debug_msg_ndebug.so odr_violation1_ndebug.so odr_violation2_ndebug.so 2>$@; \
Index: gold/testsuite/Makefile.in
===================================================================
RCS file: /cvs/src/src/gold/testsuite/Makefile.in,v
retrieving revision 1.163
diff -u -u -p -r1.163 Makefile.in
--- gold/testsuite/Makefile.in	16 Dec 2010 18:28:44 -0000	1.163
+++ gold/testsuite/Makefile.in	8 Mar 2011 00:32:26 -0000
@@ -4017,8 +4017,10 @@ uninstall-am:
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -O0 -g -c -w -o $@ $(srcdir)/debug_msg.cc
 @GCC_TRUE@@NATIVE_LINKER_TRUE@odr_violation1.o: odr_violation1.cc
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -O0 -g -c -w -o $@ $(srcdir)/odr_violation1.cc
+# Compile with different optimization flags to check that rearranged
+# instructions don't cause a false positive.
 @GCC_TRUE@@NATIVE_LINKER_TRUE@odr_violation2.o: odr_violation2.cc
-@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -O0 -g -c -w -o $@ $(srcdir)/odr_violation2.cc
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -O2 -g -c -w -o $@ $(srcdir)/odr_violation2.cc
 @GCC_TRUE@@NATIVE_LINKER_TRUE@debug_msg.err: debug_msg.o odr_violation1.o odr_violation2.o gcctestdir/ld
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	@echo $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg debug_msg.o odr_violation1.o odr_violation2.o "2>$@"
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	@if $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg debug_msg.o odr_violation1.o odr_violation2.o 2>$@; \
@@ -4032,7 +4034,7 @@ uninstall-am:
 @GCC_TRUE@@NATIVE_LINKER_TRUE@odr_violation1.so: odr_violation1.cc gcctestdir/ld
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -Bgcctestdir/ -O0 -g -shared -fPIC -w -o $@ $(srcdir)/odr_violation1.cc
 @GCC_TRUE@@NATIVE_LINKER_TRUE@odr_violation2.so: odr_violation2.cc gcctestdir/ld
-@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -Bgcctestdir/ -O0 -g -shared -fPIC -w -o $@ $(srcdir)/odr_violation2.cc
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -Bgcctestdir/ -O2 -g -shared -fPIC -w -o $@ $(srcdir)/odr_violation2.cc
 @GCC_TRUE@@NATIVE_LINKER_TRUE@debug_msg_so.err: debug_msg.so odr_violation1.so odr_violation2.so gcctestdir/ld
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	@echo $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg_so debug_msg.so odr_violation1.so odr_violation2.so "2>$@"
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	@if $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg_so debug_msg.so odr_violation1.so odr_violation2.so 2>$@; \
@@ -4046,7 +4048,7 @@ uninstall-am:
 @GCC_TRUE@@NATIVE_LINKER_TRUE@odr_violation1_ndebug.so: odr_violation1.cc gcctestdir/ld
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -Bgcctestdir/ -O0 -g0 -shared -fPIC -w -o $@ $(srcdir)/odr_violation1.cc
 @GCC_TRUE@@NATIVE_LINKER_TRUE@odr_violation2_ndebug.so: odr_violation2.cc gcctestdir/ld
-@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -Bgcctestdir/ -O0 -g0 -shared -fPIC -w -o $@ $(srcdir)/odr_violation2.cc
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -Bgcctestdir/ -O2 -g0 -shared -fPIC -w -o $@ $(srcdir)/odr_violation2.cc
 @GCC_TRUE@@NATIVE_LINKER_TRUE@debug_msg_ndebug.err: debug_msg_ndebug.so odr_violation1_ndebug.so odr_violation2_ndebug.so gcctestdir/ld
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	@echo $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg_ndebug debug_msg_ndebug.so odr_violation1_ndebug.so odr_violation2_ndebug.so "2>$@"
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	@if $(CXXLINK) -Bgcctestdir/ -Wl,--detect-odr-violations -o debug_msg_ndebug debug_msg_ndebug.so odr_violation1_ndebug.so odr_violation2_ndebug.so 2>$@; \
Index: gold/testsuite/debug_msg.cc
===================================================================
RCS file: /cvs/src/src/gold/testsuite/debug_msg.cc,v
retrieving revision 1.4
diff -u -u -p -r1.4 debug_msg.cc
--- gold/testsuite/debug_msg.cc	27 Jul 2010 08:22:49 -0000	1.4
+++ gold/testsuite/debug_msg.cc	8 Mar 2011 00:32:26 -0000
@@ -58,6 +58,10 @@ class Derived : public Base
 // This tests One Definition Rule (ODR) violations.
 void SortAscending(int array[], int size);   // in odr_violation1.cc
 void SortDescending(int array[], int size);  // in odr_violation2.cc
+// This tests One Definition Rule (ODR) non-violations.
+#include "odr_header2.h"
+OdrBase* CreateOdrDerived1();  // in odr_violation1.cc
+OdrBase* CreateOdrDerived2();  // in odr_violation2.cc
 
 extern "C" int OverriddenCFunction(int i);  // in odr_violation*.cc
 
@@ -85,5 +89,8 @@ int main()
   OverriddenCFunction(3);
   SometimesInlineFunction(3);
 
+  delete CreateOdrDerived1();
+  delete CreateOdrDerived2();
+
   return 0;
 }
Index: gold/testsuite/debug_msg.sh
===================================================================
RCS file: /cvs/src/src/gold/testsuite/debug_msg.sh,v
retrieving revision 1.10
diff -u -u -p -r1.10 debug_msg.sh
--- gold/testsuite/debug_msg.sh	27 Jul 2010 08:22:49 -0000	1.10
+++ gold/testsuite/debug_msg.sh	8 Mar 2011 00:32:26 -0000
@@ -73,18 +73,22 @@ check debug_msg.err "debug_msg.o: in fun
 
 # Check we detected the ODR (One Definition Rule) violation.
 check debug_msg.err ": symbol 'Ordering::operator()(int, int)' defined in multiple places (possible ODR violation):"
-check debug_msg.err "odr_violation1.cc:5"
-check debug_msg.err "odr_violation2.cc:5"
+check debug_msg.err "odr_violation1.cc:6"
+check debug_msg.err "odr_violation2.cc:9"
+
+# Check we don't have ODR false positives:
+check_missing debug_msg.err "OdrDerived::~OdrDerived()"
+check_missing debug_msg.err "__adjust_heap"
 # We block ODR detection for combinations of C weak and strong
 # symbols, to allow people to use the linker to override things.  We
 # still flag it for C++ symbols since those are more likely to be
 # unintentional.
 check_missing debug_msg.err ": symbol 'OverriddenCFunction' defined in multiple places (possible ODR violation):"
-check_missing debug_msg.err "odr_violation1.cc:15"
-check_missing debug_msg.err "odr_violation2.cc:17"
+check_missing debug_msg.err "odr_violation1.cc:16"
+check_missing debug_msg.err "odr_violation2.cc:20"
 check debug_msg.err ": symbol 'SometimesInlineFunction(int)' defined in multiple places (possible ODR violation):"
-check debug_msg.err "debug_msg.cc:64"
-check debug_msg.err "odr_violation2.cc:21"
+check debug_msg.err "debug_msg.cc:68"
+check debug_msg.err "odr_violation2.cc:24"
 
 # When linking together .so's, we don't catch the line numbers, but we
 # still find all the undefined variables, and the ODR violation.
@@ -92,14 +96,16 @@ check debug_msg_so.err "debug_msg.so: er
 check debug_msg_so.err "debug_msg.so: error: undefined reference to 'undef_fn2()'"
 check debug_msg_so.err "debug_msg.so: error: undefined reference to 'undef_int'"
 check debug_msg_so.err ": symbol 'Ordering::operator()(int, int)' defined in multiple places (possible ODR violation):"
-check debug_msg_so.err "odr_violation1.cc:5"
-check debug_msg_so.err "odr_violation2.cc:5"
+check debug_msg_so.err "odr_violation1.cc:6"
+check debug_msg_so.err "odr_violation2.cc:9"
+check_missing debug_msg.err "OdrDerived::~OdrDerived()"
+check_missing debug_msg.err "__adjust_heap"
 check_missing debug_msg.err ": symbol 'OverriddenCFunction' defined in multiple places (possible ODR violation):"
-check_missing debug_msg.err "odr_violation1.cc:15"
-check_missing debug_msg.err "odr_violation2.cc:17"
+check_missing debug_msg.err "odr_violation1.cc:16"
+check_missing debug_msg.err "odr_violation2.cc:20"
 check debug_msg.err ": symbol 'SometimesInlineFunction(int)' defined in multiple places (possible ODR violation):"
-check debug_msg.err "debug_msg.cc:64"
-check debug_msg.err "odr_violation2.cc:21"
+check debug_msg.err "debug_msg.cc:68"
+check debug_msg.err "odr_violation2.cc:24"
 
 # These messages shouldn't need any debug info to detect:
 check debug_msg_ndebug.err "debug_msg_ndebug.so: error: undefined reference to 'undef_fn1()'"
Index: gold/testsuite/odr_violation1.cc
===================================================================
RCS file: /cvs/src/src/gold/testsuite/odr_violation1.cc,v
retrieving revision 1.2
diff -u -u -p -r1.2 odr_violation1.cc
--- gold/testsuite/odr_violation1.cc	27 Jul 2010 08:22:49 -0000	1.2
+++ gold/testsuite/odr_violation1.cc	8 Mar 2011 00:32:26 -0000
@@ -1,4 +1,5 @@
 #include <algorithm>
+#include "odr_header1.h"
 
 class Ordering {
  public:
@@ -15,3 +16,8 @@ extern "C" int OverriddenCFunction(int i
 extern "C" int OverriddenCFunction(int i) {
   return i;
 }
+
+// Instantiate the Derived vtable, without optimization.
+OdrBase* CreateOdrDerived1() {
+  return new OdrDerived;
+}
Index: gold/testsuite/odr_violation2.cc
===================================================================
RCS file: /cvs/src/src/gold/testsuite/odr_violation2.cc,v
retrieving revision 1.2
diff -u -u -p -r1.2 odr_violation2.cc
--- gold/testsuite/odr_violation2.cc	27 Jul 2010 08:22:49 -0000	1.2
+++ gold/testsuite/odr_violation2.cc	8 Mar 2011 00:32:26 -0000
@@ -1,13 +1,16 @@
 #include <algorithm>
+#include "odr_header1.h"
 
 class Ordering {
  public:
-  bool operator()(int a, int b) {
-    // We need the "+ 1" here to force this operator() to be a
-    // different size than the one in odr_violation1.cc.
+  bool operator()(int a, int b) __attribute__((never_inline));
+};
+
+bool Ordering::operator()(int a, int b) {
+  // Optimization makes this operator() a different size than the one
+  // in odr_violation1.cc.
     return a + 1 > b + 1;
   }
-};
 
 void SortDescending(int array[], int size) {
   std::sort(array, array + size, Ordering());
@@ -21,3 +24,8 @@ extern "C" int OverriddenCFunction(int i
 int SometimesInlineFunction(int i) {
   return i * i;
 }
+
+// Instantiate the Derived vtable, with optimization (see Makefile.am).
+OdrBase* CreateOdrDerived2() {
+  return new OdrDerived;
+}

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]