[PATCH] make rich_location safe to copy

Martin Sebor msebor@gmail.com
Wed Jun 16 16:11:24 GMT 2021


On 6/16/21 9:06 AM, David Malcolm wrote:
> 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.

I see, tok->location is location_t here, and the gcc_rich_location
ctor that takes it is not declared explicit (that would prevent
the implicit conversion).

The attached patch solves the rich_location problem by a) making
the ctor explicit, b) disabling the rich_location copy ctor, c)
changing the parser to use direct initialization.  (I CC Jason
as a heads up on the C++ FE bits.)

The semi_embedded_vec should be fixed as well, regardless of this
particular use and patch.  Let me know if it's okay to commit (I'm
not open to disabling its copy ctor).

Martin

> 
> 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
>>>
>>
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-rich-location.diff
Type: text/x-patch
Size: 2626 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210616/500289b8/attachment-0001.bin>


More information about the Gcc-patches mailing list