* 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): Make OdrDerived classes.
	* 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.

1 files changed, 124 insertions, 54 deletions

diff --git a/gold/symtab.cc b/gold/symtab.cc
index d4ac297792..e882dea4e9 100644
--- a/gold/symtab.cc
+++ b/gold/symtab.cc
@@ -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,77 @@ 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;
   }
 };
 
+// Returns all of the lines attached to LOC, not just the one the
+// instruction actually came from.
+std::vector<std::string>
+Symbol_table::linenos_from_loc(const Task* task,
+                               const Symbol_location& loc)
+{
+  // 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 that records if it was ever assigned to.  This
+// allows it to be used with std::set_intersection() to check for
+// intersection rather than computing the intersection.
+struct Check_intersection
+{
+  Check_intersection()
+    : value_(false)
+  {}
+
+  bool had_intersection() const
+  { return this->value_; }
+
+  Check_intersection& operator++()
+  { return *this; }
+
+  Check_intersection& operator*()
+  { return *this; }
+
+  template<typename T>
+  Check_intersection& operator=(const T&)
+  {
+    this->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 +3123,73 @@ Symbol_table::detect_odr_violations(const Task* task,
        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();
+      const Unordered_set<Symbol_location, Symbol_location_hash>::const_iterator
+          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 = this->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 =
+              this->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());
+
+          Check_intersection intersection_result =
+              std::set_intersection(first_object_linenos.begin(),
+                                    first_object_linenos.end(),
+                                    linenos.begin(),
+                                    linenos.end(),
+                                    Check_intersection(),
+                                    Odr_violation_compare());
+          if (!intersection_result.had_intersection())
+            {
+              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.