Re: PATCH: PR gold/14675: No eh_frame info registered in exception_static_test

[Cary Coutant <ccoutant at google dot com>]

Thanks for looking at this, but I find this a bit much for what it
does -- it's pretty intrusive, and still doesn't even do the right
thing with the eh info from crt1.o (which is still placed before the
__EH_FRAME_BEGIN__ symbol, and will be ignored).

I really dislike special-cases based on filename. At one point in the
discussion, Roland suggested recognizing crtbeginT.o by the fact that
it has a relocation to the .eh_frame section symbol. I think it may be
even simpler than that, though -- given that the endcap in crtend.o
has a non-zero length, I think it's safe to put all zero-length
.eh_frame sections in front of the optimized data. That should have
the desired affect for crtbeginT.o. Let me give that a try.

-cary


On Wed, Jan 7, 2015 at 6:43 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Jan 07, 2015 at 05:16:55AM -0800, H.J. Lu wrote:
>> On Mon, Dec 22, 2014 at 09:37:38AM -0800, Cary Coutant wrote:
>> > > Here is an idea.  Does it look OK?
>> >
>> > I don't like the idea of making a file named "crtbegin" magic, and
>> > with this patch, any link *without* such a file will no longer
>> > optimize eh data.
>> >
>>
>> Here is a different approach.  We always optimize eh data unless we
>> see crti followed by crtbeginT.  If it is true, we start to optimize eh
>> data only when we see crtbeginT.  It only leaves any eh data before
>> crtbeginT unoptimized.  This patch changes the output eh data only when
>> there is crti followed by crtbeginT.  OK for trunk and 2.25 branch?
>>
>> Thanks.
>>
>>
>
> Small update.  Set has_crtbeginT_ to true only if it is false.
>
> H.J.
> --
> From 081ff0b8a64fa33a2fbd8b900f31228138400438 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Sat, 20 Dec 2014 05:45:51 -0800
> Subject: [PATCH] Treat .eh_frame section before crtbeginT as normal input
>
> Force the exception frame section from input files before the crtbeginT
> file to be handled as an ordinary input section if we aren't creating
> the exception frame header.  If we don't do this, we won't correctly
> handle the special marker symbol in the exception frame section in the
> crtbeginT file.
>
>         PR gold/14675
>         * ehframe.cc (Eh_frame::add_ehframe_input_section): Force the
>         exception frame section from input files if it can't be
>         optimized.
>         (Eh_frame::add_ehframe_input_section<32, false>): Updated.
>         (Eh_frame::add_ehframe_input_section<32, true>): Likewise.
>         (Eh_frame::add_ehframe_input_section<64, false>): Likewise.
>         (Eh_frame::add_ehframe_input_section<64, true>): Likewise.
>         * ehframe.h (Eh_frame::add_ehframe_input_section): Add a
>         bool parameter to indicate if the exception frame section
>         can be optimized.
>         * layout.cc (Layout::Layout): Initialize optimize_ehframe_ to
>         !has_crtbeginT.
>         (Layout::layout_eh_frame): Pass this->optimize_ehframe_ to
>         Eh_frame::add_ehframe_input_section.
>         (Layout::make_eh_frame_section): Set this->optimize_ehframe_
>         to true when processing the crtbeginT file if it is on command
>         line.
>         (Layout::match_file_name (const char*, const char*)): New.
>         (Layout::match_file_name(const Relobj*, const char*): Use it.
>         * layout.h (Layout::Layout): Add has_crtbeginT.
>         (Layout::match_file_name (const char*, const char*)): New.
>         (Layout): Add an optimize_ehframe_ member.
>         * main.cc (main): Pass command_line.has_crtbeginT() to layout.
>         * options.cc: Include "layout.h".
>         (Input_arguments::add_file): Set this->has_crtbeginT_ to true
>         if there is a crtbeginT file and the last one is a crti file.
>         * options.h (Input_arguments::Input_arguments): Initialize
>         has_crtbeginT_ and last_is_crti_ to false.
>         (Input_arguments::has_crtbeginT): New function.
>         (Input_arguments::has_crtbeginT_): New bool member.
>         (Input_arguments::last_is_crti_): Likewise.
>         (Command_line::has_crtbeginT): New function.
> ---
>  gold/ChangeLog  | 36 ++++++++++++++++++++++++++++++++++++
>  gold/ehframe.cc | 31 +++++++++++++++++++++----------
>  gold/ehframe.h  |  6 ++++--
>  gold/layout.cc  | 37 +++++++++++++++++++++++++++++--------
>  gold/layout.h   | 10 +++++++++-
>  gold/main.cc    |  3 ++-
>  gold/options.cc | 14 ++++++++++++++
>  gold/options.h  | 17 ++++++++++++++++-
>  8 files changed, 131 insertions(+), 23 deletions(-)
>
> diff --git a/gold/ChangeLog b/gold/ChangeLog
> index 93529fe..d2ad829 100644
> --- a/gold/ChangeLog
> +++ b/gold/ChangeLog
> @@ -1,3 +1,39 @@
> +2015-01-07  H.J. Lu  <hongjiu.lu@intel.com>
> +
> +       PR gold/14675
> +       * ehframe.cc (Eh_frame::add_ehframe_input_section): Force the
> +       exception frame section from input files if it can't be
> +       optimized.
> +       (Eh_frame::add_ehframe_input_section<32, false>): Updated.
> +       (Eh_frame::add_ehframe_input_section<32, true>): Likewise.
> +       (Eh_frame::add_ehframe_input_section<64, false>): Likewise.
> +       (Eh_frame::add_ehframe_input_section<64, true>): Likewise.
> +       * ehframe.h (Eh_frame::add_ehframe_input_section): Add a
> +       bool parameter to indicate if the exception frame section
> +       can be optimized.
> +       * layout.cc (Layout::Layout): Initialize optimize_ehframe_ to
> +       !has_crtbeginT.
> +       (Layout::layout_eh_frame): Pass this->optimize_ehframe_ to
> +       Eh_frame::add_ehframe_input_section.
> +       (Layout::make_eh_frame_section): Set this->optimize_ehframe_
> +       to true when processing the crtbeginT file if it is on command
> +       line.
> +       (Layout::match_file_name (const char*, const char*)): New.
> +       (Layout::match_file_name(const Relobj*, const char*): Use it.
> +       * layout.h (Layout::Layout): Add has_crtbeginT.
> +       (Layout::match_file_name (const char*, const char*)): New.
> +       (Layout): Add an optimize_ehframe_ member.
> +       * main.cc (main): Pass command_line.has_crtbeginT() to layout.
> +       * options.cc: Include "layout.h".
> +       (Input_arguments::add_file): Set this->has_crtbeginT_ to true
> +       if there is a crtbeginT file and the last one is a crti file.
> +       * options.h (Input_arguments::Input_arguments): Initialize
> +       has_crtbeginT_ and last_is_crti_ to false.
> +       (Input_arguments::has_crtbeginT): New function.
> +       (Input_arguments::has_crtbeginT_): New bool member.
> +       (Input_arguments::last_is_crti_): Likewise.
> +       (Command_line::has_crtbeginT): New function.
> +
>  2015-01-06  H.J. Lu  <hongjiu.lu@intel.com>
>             Cary Coutant  <ccoutant@google.com>
>
> diff --git a/gold/ehframe.cc b/gold/ehframe.cc
> index faea1a8..2fee0ff 100644
> --- a/gold/ehframe.cc
> +++ b/gold/ehframe.cc
> @@ -576,7 +576,8 @@ Eh_frame::add_ehframe_input_section(
>      section_size_type symbol_names_size,
>      unsigned int shndx,
>      unsigned int reloc_shndx,
> -    unsigned int reloc_type)
> +    unsigned int reloc_type,
> +    bool optimize_ehframe)
>  {
>    // Get the section contents.
>    section_size_type contents_len;
> @@ -595,15 +596,21 @@ Eh_frame::add_ehframe_input_section(
>      return false;
>
>    New_cies new_cies;
> -  if (!this->do_add_ehframe_input_section(object, symbols, symbols_size,
> +  bool recognized_eh_frame_section
> +    = this->do_add_ehframe_input_section(object, symbols, symbols_size,
>                                           symbol_names, symbol_names_size,
>                                           shndx, reloc_shndx,
>                                           reloc_type, pcontents,
> -                                         contents_len, &new_cies))
> +                                         contents_len, &new_cies);
> +  if (!recognized_eh_frame_section && this->eh_frame_hdr_ != NULL)
> +    this->eh_frame_hdr_->found_unrecognized_eh_frame_section();
> +
> +  // If we don't unrecognize the exception frame section or it can't
> +  // be optimized, then return false to force it to be handled as an
> +  // ordinary input section.
> +  if (!recognized_eh_frame_section
> +      || (this->eh_frame_hdr_ == NULL && !optimize_ehframe))
>      {
> -      if (this->eh_frame_hdr_ != NULL)
> -       this->eh_frame_hdr_->found_unrecognized_eh_frame_section();
> -
>        for (New_cies::iterator p = new_cies.begin();
>            p != new_cies.end();
>            ++p)
> @@ -1224,7 +1231,8 @@ Eh_frame::add_ehframe_input_section<32, false>(
>      section_size_type symbol_names_size,
>      unsigned int shndx,
>      unsigned int reloc_shndx,
> -    unsigned int reloc_type);
> +    unsigned int reloc_type,
> +    bool optimize_ehframe);
>  #endif
>
>  #ifdef HAVE_TARGET_32_BIG
> @@ -1238,7 +1246,8 @@ Eh_frame::add_ehframe_input_section<32, true>(
>      section_size_type symbol_names_size,
>      unsigned int shndx,
>      unsigned int reloc_shndx,
> -    unsigned int reloc_type);
> +    unsigned int reloc_type,
> +    bool optimize_ehframe);
>  #endif
>
>  #ifdef HAVE_TARGET_64_LITTLE
> @@ -1252,7 +1261,8 @@ Eh_frame::add_ehframe_input_section<64, false>(
>      section_size_type symbol_names_size,
>      unsigned int shndx,
>      unsigned int reloc_shndx,
> -    unsigned int reloc_type);
> +    unsigned int reloc_type,
> +    bool optimize_ehframe);
>  #endif
>
>  #ifdef HAVE_TARGET_64_BIG
> @@ -1266,7 +1276,8 @@ Eh_frame::add_ehframe_input_section<64, true>(
>      section_size_type symbol_names_size,
>      unsigned int shndx,
>      unsigned int reloc_shndx,
> -    unsigned int reloc_type);
> +    unsigned int reloc_type,
> +    bool optimize_ehframe);
>  #endif
>
>  } // End namespace gold.
> diff --git a/gold/ehframe.h b/gold/ehframe.h
> index aa2bd31..73e0fd4 100644
> --- a/gold/ehframe.h
> +++ b/gold/ehframe.h
> @@ -371,7 +371,8 @@ class Eh_frame : public Output_section_data
>    // is the relocation section if any (0 for none, -1U for multiple).
>    // RELOC_TYPE is the type of the relocation section if any.  This
>    // returns whether the section was incorporated into the .eh_frame
> -  // data.
> +  // data.  OPTIMIZE_EHFRAME is true if the exception frame section
> +  // can be optimized.
>    template<int size, bool big_endian>
>    bool
>    add_ehframe_input_section(Sized_relobj_file<size, big_endian>* object,
> @@ -380,7 +381,8 @@ class Eh_frame : public Output_section_data
>                             const unsigned char* symbol_names,
>                             section_size_type symbol_names_size,
>                             unsigned int shndx, unsigned int reloc_shndx,
> -                           unsigned int reloc_type);
> +                           unsigned int reloc_type,
> +                           bool optimize_ehframe);
>
>    // Add a CIE and an FDE for a PLT section, to permit unwinding
>    // through a PLT.  The FDE data should start with 8 bytes of zero,
> diff --git a/gold/layout.cc b/gold/layout.cc
> index acc03b2..1ef4d18 100644
> --- a/gold/layout.cc
> +++ b/gold/layout.cc
> @@ -418,7 +418,8 @@ Layout_task_runner::run(Workqueue* workqueue, const Task* task)
>
>  // Layout methods.
>
> -Layout::Layout(int number_of_input_files, Script_options* script_options)
> +Layout::Layout(int number_of_input_files, Script_options* script_options,
> +              bool has_crtbeginT)
>    : number_of_input_files_(number_of_input_files),
>      script_options_(script_options),
>      namepool_(),
> @@ -469,6 +470,7 @@ Layout::Layout(int number_of_input_files, Script_options* script_options)
>      unique_segment_for_sections_specified_(false),
>      incremental_inputs_(NULL),
>      record_output_section_data_from_script_(false),
> +    optimize_ehframe_(!has_crtbeginT),
>      script_output_section_data_list_(),
>      segment_states_(NULL),
>      relaxation_debug_check_(NULL),
> @@ -1428,7 +1430,8 @@ Layout::layout_eh_frame(Sized_relobj_file<size, big_endian>* object,
>                                                          symbol_names_size,
>                                                          shndx,
>                                                          reloc_shndx,
> -                                                        reloc_type))
> +                                                        reloc_type,
> +                                                        this->optimize_ehframe_))
>      {
>        os->update_flags_for_input_section(shdr.get_sh_flags());
>
> @@ -1485,6 +1488,14 @@ Layout::make_eh_frame_section(const Relobj* object)
>    if (os == NULL)
>      return NULL;
>
> +  // optimize_ehframe_ is false only if there is a crtbeginT file on
> +  // command line.  We can't optimize the exception frame section
> +  // until we have seen the crtbeginT file.
> +  if (!this->optimize_ehframe_
> +      && object != NULL
> +      && Layout::match_file_name(object, "crtbeginT"))
> +    this->optimize_ehframe_ = true;
> +
>    if (this->eh_frame_section_ == NULL)
>      {
>        this->eh_frame_section_ = os;
> @@ -5092,19 +5103,18 @@ Layout::output_section_name(const Relobj* relobj, const char* name,
>    return name;
>  }
>
> -// Return true if RELOBJ is an input file whose base name matches
> -// FILE_NAME.  The base name must have an extension of ".o", and must
> -// be exactly FILE_NAME.o or FILE_NAME, one character, ".o".  This is
> +// Return true if FILE is an input file whose base name matches
> +// MATCH.  The base name must have an extension of ".o", and must
> +// be exactly MATCH.o or MATCH, one character, ".o".  This is
>  // to match crtbegin.o as well as crtbeginS.o without getting confused
>  // by other possibilities.  Overall matching the file name this way is
>  // a dreadful hack, but the GNU linker does it in order to better
>  // support gcc, and we need to be compatible.
>
>  bool
> -Layout::match_file_name(const Relobj* relobj, const char* match)
> +Layout::match_file_name(const char* file, const char* match)
>  {
> -  const std::string& file_name(relobj->name());
> -  const char* base_name = lbasename(file_name.c_str());
> +  const char* base_name = lbasename(file);
>    size_t match_len = strlen(match);
>    if (strncmp(base_name, match, match_len) != 0)
>      return false;
> @@ -5114,6 +5124,17 @@ Layout::match_file_name(const Relobj* relobj, const char* match)
>    return memcmp(base_name + base_len - 2, ".o", 2) == 0;
>  }
>
> +// Return true if FILE is an input file whose base name matches
> +// MATCH.  The base name must have an extension of ".o", and must
> +// be exactly MATCH.o or MATCH, one character, ".o".
> +
> +bool
> +Layout::match_file_name(const Relobj* relobj, const char* match)
> +{
> +  const std::string& file_name(relobj->name());
> +  return Layout::match_file_name(file_name.c_str(), match);
> +}
> +
>  // Check if a comdat group or .gnu.linkonce section with the given
>  // NAME is selected for the link.  If there is already a section,
>  // *KEPT_SECTION is set to point to the existing section and the
> diff --git a/gold/layout.h b/gold/layout.h
> index aec0c53..bf4a44a 100644
> --- a/gold/layout.h
> +++ b/gold/layout.h
> @@ -488,7 +488,7 @@ enum Output_section_order
>  class Layout
>  {
>   public:
> -  Layout(int number_of_input_files, Script_options*);
> +  Layout(int number_of_input_files, Script_options*, bool has_crtbeginT);
>
>    ~Layout()
>    {
> @@ -757,6 +757,12 @@ class Layout
>             || strncmp(name, ".stab", sizeof(".stab") - 1) == 0);
>    }
>
> +  // Return true if FILE is an input file whose base name matches
> +  // FILE_NAME.  The base name must have an extension of ".o", and
> +  // must be exactly FILE_NAME.o or FILE_NAME, one character, ".o".
> +  static bool
> +  match_file_name(const char* file, const char* file_name);
> +
>    // Return true if RELOBJ is an input file whose base name matches
>    // FILE_NAME.  The base name must have an extension of ".o", and
>    // must be exactly FILE_NAME.o or FILE_NAME, one character, ".o".
> @@ -1420,6 +1426,8 @@ class Layout
>    Incremental_inputs* incremental_inputs_;
>    // Whether we record output section data created in script
>    bool record_output_section_data_from_script_;
> +  // Whether to optimize the exception frame section.
> +  bool optimize_ehframe_;
>    // List of output data that needs to be removed at relaxation clean up.
>    Output_section_data_list script_output_section_data_list_;
>    // Structure to save segment states before entering the relaxation loop.
> diff --git a/gold/main.cc b/gold/main.cc
> index c5e50d6..a3aea4b 100644
> --- a/gold/main.cc
> +++ b/gold/main.cc
> @@ -227,7 +227,8 @@ main(int argc, char** argv)
>
>    // The layout object.
>    Layout layout(command_line.number_of_input_files(),
> -               &command_line.script_options());
> +               &command_line.script_options(),
> +               command_line.has_crtbeginT());
>
>    if (layout.incremental_inputs() != NULL)
>      layout.incremental_inputs()->report_command_line(argc, argv);
> diff --git a/gold/options.cc b/gold/options.cc
> index 7eb8f27..12c1aaa 100644
> --- a/gold/options.cc
> +++ b/gold/options.cc
> @@ -38,6 +38,7 @@
>  #include "script.h"
>  #include "target-select.h"
>  #include "options.h"
> +#include "layout.h"
>  #include "plugin.h"
>
>  namespace gold
> @@ -1346,6 +1347,19 @@ Input_arguments::add_file(Input_file_argument& file)
>        return this->input_argument_list_.back().lib()->add_file(file);
>      }
>    this->input_argument_list_.push_back(Input_argument(file));
> +  if (!this->has_crtbeginT_)
> +    {
> +      // Set has_crtbeginT_ to true only if the last one is a crti file.
> +      if (Layout::match_file_name(file.name(), "crti"))
> +       this->last_is_crti_ = true;
> +      else
> +       {
> +         if (this->last_is_crti_
> +             && Layout::match_file_name(file.name(), "crtbeginT"))
> +           this->has_crtbeginT_ = true;
> +         this->last_is_crti_ = false;
> +       }
> +    }
>    return this->input_argument_list_.back();
>  }
>
> diff --git a/gold/options.h b/gold/options.h
> index 956a7f4..83bdd20 100644
> --- a/gold/options.h
> +++ b/gold/options.h
> @@ -1970,7 +1970,8 @@ class Input_arguments
>    typedef Input_argument_list::const_iterator const_iterator;
>
>    Input_arguments()
> -    : input_argument_list_(), in_group_(false), in_lib_(false), file_count_(0)
> +    : input_argument_list_(), in_group_(false), in_lib_(false),
> +      file_count_(0), has_crtbeginT_(false), last_is_crti_(false)
>    { }
>
>    // Add a file.
> @@ -2030,11 +2031,20 @@ class Input_arguments
>    number_of_input_files() const
>    { return this->file_count_; }
>
> +  // Return whether there is a crtbeginT file.
> +  bool
> +  has_crtbeginT() const
> +  { return this->has_crtbeginT_; }
> +
>   private:
>    Input_argument_list input_argument_list_;
>    bool in_group_;
>    bool in_lib_;
>    unsigned int file_count_;
> +  // Whether there is a crtbeginT file.
> +  bool has_crtbeginT_;
> +  // Whether the last one is a crti file.
> +  bool last_is_crti_;
>  };
>
>
> @@ -2103,6 +2113,11 @@ class Command_line
>    end() const
>    { return this->inputs_.end(); }
>
> +  // Whether there is a crtbeginT file.
> +  bool
> +  has_crtbeginT() const
> +  { return this->inputs_.has_crtbeginT(); }
> +
>   private:
>    Command_line(const Command_line&);
>    Command_line& operator=(const Command_line&);
> --
> 1.9.3
>