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] gold: Implement --exclude-libs option


"Doug Kwan (éæå)" <dougkwan@google.com> writes:

> 2009-05-19  Doug Kwan  <dougkwan@google.com>
>
>         * archive.cc (Archive::get_elf_object_for_member): Set no_export
>         flag of object.
>         * archive.h (option.h, parameters.h): New includes.
>         (Archive::Archive): Set no_explort_.
>         (Archive::no_export): New method.
>         (Archive::no_export_): New field.
>         * object.h (Object::Object): Initialize no_export_ to false.
>         (Object::no_export, Object::set_no_export): New methods.
>         (Object::no_export_): New field.
>         * options.cc (General_options::parse_exclude_libs): New method.
>         (General_options::check_excluded_libs) Same.
>         * options.h (exclude_libs): New option.
>         (General_options::check_excluded_libs): New method declaration.
>         (General_options::excluded_libs_): New field.
>         * symtab.cc (Symbol_table::add_from_relobj): Hide symbols with
>         default or protected visibility if an object has no-export flag set.
>         testsuite/Makefile.am (check_PROGRAMS): Add exclude_lib_test.
>         (check_SCRIPTS): Add exclude_libs_test.sh.
>         (check_DATA): Add exclude_libs_test.syms.
>         (MOSTLYCLEANFILES): Add exlude_libs_test.syms,
>         libexclude_libs_test_1.a and libexclude_libs_test_2.a.
>         (exclude_libs_test_SOURCES, exclude_libs_test_DEPENDENDICES,
>         exclude_libs_test_LDFLAGS and exclude_libs_test_LDADD): Define.
>         (exclude_libs_test.syms, libexclude_libs_test_1.a,
>         libexclude_libs_test_2.a): New rules.
>         * testsuite/Makefile.in: Regenerate.
>         * testsuite/exclude_libs_test.c: New file.
>         * testsuite/exclude_libs_test_1.c: Ditto.
>         * testsuite/exclude_libs_test_2.c: Ditto.
>         * testsuite/exclude_libs_test_2.sh: Ditto.




> diff -Nrpu gold-cvs-3/src/gold/archive.cc gold-cvs-2/src/gold/archive.cc
> --- gold-cvs-3/src/gold/archive.cc	2009-03-24 11:42:10.000000000 -0700
> +++ gold-cvs-2/src/gold/archive.cc	2009-05-18 16:42:37.360972000 -0700
> @@ -549,10 +549,12 @@ Archive::get_elf_object_for_member(off_t
>        return NULL;
>      }
>  
> -  return make_elf_object((std::string(this->input_file_->filename())
> -			  + "(" + member_name + ")"),
> -			 input_file, memoff, ehdr, read_size,
> -			 punconfigured);
> +  Object *obj = make_elf_object((std::string(this->input_file_->filename())
> +				 + "(" + member_name + ")"),
> +				input_file, memoff, ehdr, read_size,
> +				punconfigured);
> +  obj->set_no_export(no_export());
> +  return obj;

Gold is heavily templated, and in templates name lookup can be
unexpected.  Therefore, gold code consistently refers to local fields
and methods using "this->".  This is particular important for methods,
which do not have the trailing underscore.  So, this should me
  obj->set_no_export(this->no_export());


> diff -Nrpu gold-cvs-3/src/gold/archive.h gold-cvs-2/src/gold/archive.h
> --- gold-cvs-3/src/gold/archive.h	2009-03-13 22:56:46.000000000 -0700
> +++ gold-cvs-2/src/gold/archive.h	2009-05-19 00:06:45.165003000 -0700
> @@ -27,6 +27,8 @@
>  #include <vector>
>  
>  #include "fileread.h"
> +#include "options.h"
> +#include "parameters.h"

Instead of doing this, just move the constructor into archive.cc.

> @@ -54,7 +56,10 @@ class Archive
>        extended_names_(), armap_checked_(), seen_offsets_(), members_(),
>        is_thin_archive_(is_thin_archive), included_member_(false),
>        nested_archives_(), dirpath_(dirpath), task_(task), num_members_(0)
> -  { }
> +  {
> +    no_export_ =
> +      parameters->options().check_excluded_libs(input_file->found_name());
> +  }

this->no_export_ = ...

> +void
> +General_options::parse_exclude_libs(const char*, const char* arg,
> +                                    Command_line*)
> +{
> +  const char *p = arg, *end;
> +
> +  while (*p != '\0')
> +    {
> +      end = strpbrk(p, ",:");
> +      if (end == NULL)
> +        end = p + strlen(p);
> +      size_t length = end - p;
> +      std::string s(p, length);
> +      excluded_libs_.insert(s);
> +      if (*end == '\0')
> +        break;
> +      p = end + 1;
> +    }
> +}

This function needs a comment.  In particular, the comment should say
that the implementation is based on what the GNU linker does.

This is pretty minor, but this code would be simpler if you used strcspn
instead of strpbrk.
  while (*p != '\0')
    {
      size_t length = strcspn(p, ",:");
      this->excluded_libs_.insert(std::string(p, length));
      p += length;
    }


> +bool
> +General_options::check_excluded_libs (const std::string &name) const

This function needs a comment referring to the GNU linker, since
otherwise the behaviour seems kind of weird.


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]