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]

Re: [PATCH] Fix the ODR checker again


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


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