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 6/7] [gold] Add initial source code for dwp utility.


>> This patch adds the new source files for the dwp utility itself, and updates
>> the gold Makefiles to build it.
>
>> -noinst_PROGRAMS = ld-new incremental-dump
>> +noinst_PROGRAMS = ld-new incremental-dump dwp
>>  noinst_LIBRARIES = libgold.a
>
> Are we going to want to install dwp at some point?  Presumably, unlike
> incremental-dump, we expect other packages to use it.

Yes, I've made that change already. I left dwp in noinst_PROGRAMS, and
added the commands to the install-exec-local rule -- I guess I could
just add a "bin_PROGRAMS = dwp" statement instead and let automake
take care of it?

>> +dwp_SOURCES = dwp.cc
>> +dwp_DEPENDENCIES = libgold.a $(LIBIBERTY) $(LIBINTL_DEP)
>> +dwp_LDADD = libgold.a $(LIBIBERTY) $(LIBINTL) $(THREADSLIB) $(LIBDL)
>
> In practice the same arguments that caused us to add $(GOLD_LDADD) and
> $(GOLD_LDFLAGS) are going to apply here.  $(GOLD_LDADD) should be
> added to dwp_LDADD and $(GOLD_LDFLAGS should be in a new dwp_LDFLAGS
> variable.

OK. Makes sense.

>> +// An ELF input file.
>> +// We derive from Sized_relobj so that we can use interfaces
>> +// in libgold to access the file.
>
> Did you consider deriving from Sized_relobj_file?  That would let you
> skip defining some of the unreachable virtual funtions.  It would also
> give you an Elf_file accessible via elf_file().

I did consider that, but I wanted to avoid bringing in object.o. I
failed in that, but I think a little more restructuring could still
eliminate it, and further reduce the number of .o files from libgold.a
that are needed for dwp. For example, just linking in errors.o forces
in object.o because of error_at_location().

I also considered abstracting yet another layer between Relobj and
Sized_relobj, or between Object and Relobj, so that I could have some
basic functions for accessing a Relobj without all the linker-specific
stuff. Multiple inheritance would probably have helped here, too --
for Dwarf_info_reader, all I need is a small subset of the interfaces
from Relobj. What's your policy on multiple inheritance --
specifically in the case where a second base class would be pure
interface? (This would probably be much easier in Go. (Too bad the
name "gold" is already taken.))

>> + private:
>> +  // General access to the ELF file.
>> +  elfcpp::Elf_file<size, big_endian, Object> elf_file_;
>> +
>> +};
>
> Extra blank line before }.

OK.

>> +// Return the location of the contents of a section.
>> +
>> +template <int size, bool big_endian>
>> +const unsigned char*
>> +Sized_relobj_dwo<size, big_endian>::do_section_contents(
>> +    unsigned int shndx,
>> +    section_size_type* plen,
>> +    bool cache)
>
> The comment seems to be wrong: this returns the section contents, not
> the location.

I think "location" means "pointer to" here. In object.h, we seem to
alternate between "Return a view of the contents of a section" and
"Return the location of the contents of a section". This was just
copy/pasted from object.h.

>> +  // Collect file names and options.
>> +  typedef std::vector<char*> File_list;
>> +  File_list files;
>> +  const char* output_filename = NULL;
>> +  bool no_more_options = false;
>> +  bool verbose = false;
>> +  for (int i = 1; i < argc; ++i)
>> +    {
>> +      if (no_more_options || argv[i][0] != '-')
>> +       files.push_back(argv[i]);
>
> Use getopt here.  gold can't easily use getopt because it cares about
> the ordering of option and non-option arguments.  But this program can
> use getopt.  There are examples over in the binutils directory.

OK. I thought about using getopt, but since gold wasn't using it, and
the options are simple, I went with this. No problem to change it,
though.

>> +#ifdef ENABLE_NLS
>> +# include <libintl.h>
>> +# define _(String) gettext (String)
>> +# ifdef gettext_noop
>> +#  define N_(String) gettext_noop (String)
>> +# else
>> +#  define N_(String) (String)
>> +# endif
>> +#else
>> +# define gettext(Msgid) (Msgid)
>> +# define dgettext(Domainname, Msgid) (Msgid)
>> +# define dcgettext(Domainname, Msgid, Category) (Msgid)
>> +# define textdomain(Domainname) do {} while (0) /* nothing */
>> +# define bindtextdomain(Domainname, Dirname) do {} while (0) /* nothing */
>> +# define _(String) (String)
>> +# define N_(String) (String)
>> +#endif
>> +
>> +// Figure out how to get a hash set and a hash map.
>
> Let's pull this stuff copied from gold.h into a shared header file,
> perhaps system.h.

OK, sounds good.

I also found a narrowing conversion with a more recent GCC where I
initialize a Section::len (an off_t) with a section_size_type. I've
converted that over to use section_size_type.

I'll try to have an updated patch later today.

-cary


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