This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 08/16] libiberty.h: Provide a CLEAR_VAR macro
- From: David Malcolm <dmalcolm at redhat dot com>
- To: DJ Delorie <dj at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org, binutils at sourceware dot org
- Date: Mon, 01 Jun 2015 21:16:38 -0400
- Subject: Re: [PATCH 08/16] libiberty.h: Provide a CLEAR_VAR macro
- Authentication-results: sourceware.org; auth=none
- References: <1433192664-50156-1-git-send-email-dmalcolm at redhat dot com> <1433192664-50156-9-git-send-email-dmalcolm at redhat dot com> <201506012147 dot t51Ll08P015579 at greed dot delorie dot 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