This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Fix the ODR checker again
- From: Ian Lance Taylor <iant at google dot com>
- To: Jeffrey Yasskin <jyasskin at google dot com>
- Cc: binutils at sourceware dot org, Cary Coutant <ccoutant at google dot com>
- Date: Tue, 08 Mar 2011 17:12:11 -0800
- Subject: Re: [PATCH] Fix the ODR checker again
- References: <AANLkTinCJvt93_H1hVmqm8eifZy_PakPNZOHsJBxeG1c@mail.gmail.com>
Jeffrey Yasskin <jyasskin@google.com> writes:
> 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.
Note that lines in ChangeLog should be wrapped at 72 columns or so.
> -// 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 "";
This function should have a comment.
> @@ -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);
Write this->format_file_lineno.
> + 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));
Write this->format_file_lineno.
> +// 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
> +{
Add a blank line after the comment line before the start of the
function.
> @@ -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;
Please capitalize the comment. Comments should in general be full
sentences, although that kind slide for the comments above which are on
the fields.
Make line_num have type unsigned int, it should make the code more
efficient.
> + // 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;
Capitalize this comment too.
> +std::vector<std::string>
> +Symbol_table::linenos_from_loc(const Task* task,
> + const Symbol_location& loc) const
> +{
This function should have a comment.
> +// 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)
> + {}
Put the ": value_(false)" on the next line, indented two spaces. See
other constructors.
> + 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_;
> +};
Using an output iterator is cute but personally I think using operator
bool() is gilding the lily. It's far from obvious why you can write
!std::set_intersection below. How about just using a regular accessor,
and using a local variable for the result of std::set_interaction.
> + 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();
gold never initializes two variables on one line like this. It's too
easy to miss the second initialization. Use a second line. use a
typedef if you want.
> + first_object_name = locs->object->name();
> + first_object_linenos = linenos_from_loc(task, *locs);
Write this->lineos_from_loc.
> + struct Symbol_location; // See definition below.
Just move the definition up.
This is OK with those changes.
Thanks.
Ian