Re: [PATCH 5/5] add libcc1

[Tom Tromey <tromey@redhat.com>]

>>>>> "Trevor" == Trevor Saunders <tsaunders@mozilla.com> writes:

Trevor> I'm curious, what is the reason you choose not to write this in C++11 or
Trevor> later?  Its distributed with gcc, so the only case where you aren't
Trevor> building with the in tree compiler and libraries is when your cross
Trevor> compiling gcc, and it doesn't seem particularly important to support
Trevor> building the plugin or library in that configuration.  So istm you could
Trevor> go all the way and assume you are being built with trunk gcc and
Trevor> libraries.

The plugin has to be ABI compatible with GCC itself, and my
understanding was that C++11 and "however GCC is built" are not
necessarily compatible.

Switching to C++11 would be an improvement -- variadic templates would
simplify the RPC code (with a complicated caveat).  So if it is possible
I am interested.

Trevor> I'm going to use this as an excuse to bring up something I've wanted to
Trevor> discuss for a while.

Trevor> So can we add C++ stuff to libiberty and allow building
Trevor> libiberty without it for binutils / gdb, or can we do something
Trevor> else to avoid this kind of stuff?

One way would be to just make a new top-level directory for a new
library.

Trevor> This question also arises in the case of templating splay_tree, and I
Trevor> imagine if gdb switches to C++ some day they'll want to reuse vec.h.

While I would like that to happen, I think the odds are very long now.

>> +    connection (int fd)
>> +      : m_fd (fd),
>> +	m_aux_fd (-1),
>> +	m_callbacks ()

Trevor>  Personally I'd leave that to the compiler to write, but I guess there's
Trevor>  something to be said for being explicit.

I can't recall if I did this in response to a warning or if it was just
because I wanted to be explicit.

I'm inclined to leave it, but I suppose only out of inertia.

>> +  void print (const char *buf)

Trevor> explicitly mark it as virtual?

Good idea, done.

>> +// This is a wrapper function that is called by the RPC system and
>> +// that then forwards the call to the library user.  Note that the
>> +// return value is not used; the type cannot be 'void' due to
>> +// limitations in our simple RPC.
>> +gcc_address

Trevor> looks like this one probably is used?

Thanks, fixed.

>> +  char **argv = new (std::nothrow) char *[self->args.size () + 1];

Trevor> What's the point of making this no throw? you don't null check it so
Trevor> you'll crash anyway afaict.

Thanks.  I changed it to do a NULL check.  It's nothrow because nothing
in libcc1 or gdb is prepared for a C++ exception.  While I like
exceptions (gdb uses its own longjmp-based exception system
extensively), my understanding is that they aren't currently used in
gcc.

>> +cc1_plugin::status
>> +cc1_plugin::unmarshall (connection *conn, char **result)
>> +{
>> +  unsigned long long len;
>> +
>> +  if (!conn->require ('s'))
>> +    return FAIL;
>> +  if (!conn->get (&len, sizeof (len)))
>> +    return FAIL;
>> +
>> +  if (len == -1ULL)
>> +    {
>> +      *result = NULL;
>> +      return OK;
>> +    }
>> +
>> +  char *str = new (std::nothrow) char[len + 1];

Trevor> It'd be really nice if the type of the out arg forced the caller to deal
Trevor> with deleting the string like unique_ptr<char>, it would be even nicer
Trevor> if you could stick a random buffer in a std::string, but I guess you
Trevor> can't :(

Yeah, it's all quite simplistic.  I suppose it could be upgraded, there
just didn't seem to be a need.

Trevor> Also where does this array get deleted?

The unmarshalling methods are generally called via argument_wrappers.
An example is in connection.cc:

		// Use an argument_wrapper here to simplify management
		// of the string's lifetime.
		argument_wrapper<char *> method_name;

		if (!method_name.unmarshall (this))
		  return FAIL;

Then in rpc.hh:

  // Specialization for string types.
  template<>
  class argument_wrapper<const char *>
  {
  public:
    argument_wrapper () : m_object (NULL) { }
    ~argument_wrapper ()
    {
      delete[] m_object;
    }

Tom