[PATCH] make rich_location safe to copy

David Malcolm dmalcolm@redhat.com
Wed Jun 16 15:06:41 GMT 2021


On Wed, 2021-06-16 at 08:52 -0600, Martin Sebor wrote:
> On 6/16/21 6:38 AM, David Malcolm wrote:
> > On Tue, 2021-06-15 at 19:48 -0600, Martin Sebor wrote:
> > 
> > Thanks for writing the patch.
> > 
> > > While debugging locations I noticed the semi_embedded_vec template
> > > in line-map.h doesn't declare a copy ctor or copy assignment, but
> > > is being copied in a couple of places in the C++ parser (via
> > > gcc_rich_location).  It gets away with it most likely because it
> > > never grows beyond the embedded buffer.
> > 
> > Where are these places?  I wasn't aware of this.
> 
> They're in the attached file along with the diff to reproduce
> the errors.

Thanks.

Looks like:

   gcc_rich_location richloc = tok->location;

is implicitly constructing a gcc_rich_location, then copying it to
richloc.  This should instead be simply:

   gcc_rich_location richloc (tok->location);

which directly constructs the richloc in place, as I understand it.

Dave

> 
> I was seeing strange behavior in my tests that led me to rich_location
> and the m_ranges member.  The problem turned out to be unrelated but
> before I figured it out I noticed the missing copy ctor and deleted
> it to see if it was being used.  Since that's such a pervasive bug
> in GCC code (and likely elsewhere as well) I'm thinking I should take
> the time to develop the warning I've been thinking about to detect it.
> 
> 
> > > The attached patch defines the copy ctor and also copy assignment
> > > and adds the corresponding move functions.
> > 
> > Note that rich_location::m_fixit_hints "owns" the fixit_hint
> > instances,
> > manually deleting them in rich_location's dtor, so simply doing a
> > shallow copy of it would be wrong.
> > 
> > Also, a rich_location stores other pointers (to range_labels and
> > diagnostic_path), which are borrowed pointers, where their lifetime
> > is
> > assumed to outlive any (non-dtor) calls to the rich_location.  So I'm
> > nervous about code that copies rich_location instances.
> > 
> > I think I'd prefer to forbid copying them; what's the use-case for
> > copying them?  Am I missing something here?
> 
> I noticed and fixed just the one problem I uncovered by accident with
> the missing copy ctor.  If there are others I don't know about them.
> Preventing code from copying rich_location might make sense
> independently of fixing the vec class to be safely copyable.
> 
> Martin
> 
> > 
> > > 
> > > Tested on x86_64-linux.
> > > 
> > > Martin
> > 
> > Thanks
> > Dave
> > 
> 




More information about the Gcc-patches mailing list