Re: [PATCH 08/16] libiberty.h: Provide a CLEAR_VAR macro

[David Malcolm <dmalcolm@redhat.com>]

On Mon, 2015-06-01 at 17:47 -0400, DJ Delorie wrote:
> > +/* Fill an lvalue with zero bits.  */
> > +#define CLEAR_VAR(S) \
> > +  do { memset (&(S), 0, sizeof (S)); } while (0)
> 
> Hmmm... I don't see the point in this.  The macro is almost as long as
> a bare memset() would be, and replaces a well-known function with
> something unknown outside this project.  It neither hides a bug in an
> OS nor provides a common way to handle a difficult task across
> platforms.

Thanks for looking at this.

FWIW I'm not in love with the name of the macro, but I find it useful.
In the initial version of patches 10 and 12 (state purging within "gas"
and "ld" subdirs) I didn't have this macro, and had about 30 memset
invocations.

There are a couple of ways to mess up such code; consider:

  memset (&statement_list, 0, sizeof (statement_list));
  memset (&stat_save, 0, sizeof (statement_list));

where a bug is hiding: the 2nd memset is using the wrong sizeof, leading
to only part of it being zeroed.  Having a macro for this makes the code
shorter:

  CLEAR_VAR (statement_list);
  CLEAR_VAR (stat_save);

and thus more reliable (my eyes glaze over looking at all the memsets in
the more involved examples).

There's probably another way to mess this up, by forgetting the & on the
first param.

> You also do NOT want to use memset to zero out a C++ structure that
> contains more than just "plain old data" - you could easily corrupt
> the structure's hidden data.

Are you thinking of vtables here, "public" vs "private", or of
inheritance?  FWIW I'm only using this from within binutils, which AFAIK
is pure C (I'm new to binutils).

> Also, pedantically speaking, "all bits zero" is not guaranteed to be
> the same as float 0.0, double 0.0, or pointer (void *)0.

The purpose of the code is to restore the state back to what it was when
the library was first loaded; in the various foo_c_finalize () functions
in patches 10 and 12 I try to only use CLEAR_VAR for structs, and
instead spell out NULL etc for ptrs.  Perhaps CLEAR_STRUCT () might be a
better name?  Also, if this isn't so much a libiberty thing (not being
an OS-compatibility thing), is there a place to put it in internal
support headers?  (in the sense that this would be an implementation
detail, used by both gas and ld)

Thanks
Dave