From: Jeffrey Yasskin <jyasskin@google.com>
To: binutils@sourceware.org
Subject: Re: [PATCH] Loosen the Gold ODR checker to only compare filenames
Date: Fri, 04 Feb 2011 19:24:00 -0000 [thread overview]
Message-ID: <AANLkTi=Qwtg4spK0xL1LygFPkGzwbz2M4fn8D8eexTx0@mail.gmail.com> (raw)
In-Reply-To: <AANLkTikvkDwyk7K_M1Dgk9cgUyU=vKA6D8-UUncPXs6L@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]
Here's a tweaked version in response to Ian's review.
On Thu, Feb 3, 2011 at 3:09 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
> This patch should do a better job. I went back to the
> substring-comparison the original code used, and chopped off the line
> number in addition to the directory. The code to remove the directory
> is different because the original code wasn't a transitive relation:
> it reported that "c:1" < "d/a:1" < "a/b:1" < "c:1".
>
> 2011-02-03 Jeffrey Yasskin <jyasskin@google.com>
>
> * symtab.cc: Sort by just the filename.
>
> On Thu, Feb 3, 2011 at 2:10 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>> Whoops, please don't submit this. The strings that Source_location
>> points at don't live long enough to use them in the way I'm using
>> them. I'll send a working patch shortly.
>>
>> On Wed, Feb 2, 2011 at 10:44 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>>> Code compiled with different flags, especially -O, may have a
>>> different line number for the first instruction in a function. This
>>> produces false positives in the ODR checker when linking object files
>>> that should be ABI-compatible.
>>>
>>> The best fixes would be to A) look at the DW_AT_decl_line of the
>>> DW_TAG_subprogram for the function, but this would require gold to
>>> parse a whole new debug section, or B) hash the ODR-relevant aspects
>>> of each function into a new dwarf attribute, but this would require
>>> gcc to produce the hash and gold to parse a whole new debug section.
>>>
>>> Instead, loosening the ODR check to allow a function's definitions to
>>> be from anywhere within the same file removes the false positives with
>>> much less work, and would have caused very few extra false negatives
>>> in Google's codebase.
>>>
>>> 2011-02-02 Jeffrey Yasskin <jyasskin@google.com>
>>>
>>> * dwarf_reader.h: Add a Source_location type, and change the
>>> addr2line functions to return it.
>>> * dwarf_reader.cc: Implement Source_location, and change the
>>> addr2line functions to return it.
>>> * symtab.cc: Sort by just the filename.
>>> * object.cc: Convert a Source_location return to a std::string.
>>>
>>
>
[-- Attachment #2: looser_odr_check.patch --]
[-- Type: text/x-patch, Size: 2615 bytes --]
Index: gold/symtab.cc
===================================================================
RCS file: /cvs/src/src/gold/symtab.cc,v
retrieving revision 1.146
diff -u -r1.146 symtab.cc
--- gold/symtab.cc 24 Jan 2011 21:48:40 -0000 1.146
+++ gold/symtab.cc 4 Feb 2011 19:19:49 -0000
@@ -3012,11 +3012,12 @@
// 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
-// location, we consider instances with the same base filename and
-// line number to be the same. This is because different object
-// files/shared libraries can include the same header file using
-// different paths, and we don't want to report an ODR violation in
-// that case.
+// 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
+// different optimization settings can make the line number appear to
+// be a couple lines off, and we don't want to report an ODR violation
+// in those cases.
// This struct is used to compare line information, as returned by
// Dwarf_line_info::one_addr2line. It implements a < comparison
@@ -3027,13 +3028,28 @@
bool
operator()(const std::string& s1, const std::string& s2) const
{
- std::string::size_type pos1 = s1.rfind('/');
- std::string::size_type pos2 = s2.rfind('/');
- if (pos1 == std::string::npos
- || pos2 == std::string::npos)
- return s1 < s2;
- return s1.compare(pos1, std::string::npos,
- s2, pos2, std::string::npos) < 0;
+ // Inputs should be of the form "dirname/filename:linenum" where
+ // "dirname/" is optional. We want to compare just the filename.
+
+ // Find the last '/' and ':' 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;
}
};
next prev parent reply other threads:[~2011-02-04 19:24 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-02-03 6:45 Jeffrey Yasskin
2011-02-03 22:11 ` Jeffrey Yasskin
2011-02-03 23:09 ` Jeffrey Yasskin
2011-02-04 19:24 ` Jeffrey Yasskin [this message]
2011-02-04 22:08 ` Ian Lance Taylor
2011-02-05 1:56 ` Ian Lance Taylor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='AANLkTi=Qwtg4spK0xL1LygFPkGzwbz2M4fn8D8eexTx0@mail.gmail.com' \
--to=jyasskin@google.com \
--cc=binutils@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).