[PATCH] make rich_location safe to copy
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.
> 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).
>> 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
>>> 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
>>> 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.
>>>> Tested on x86_64-linux.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 2626 bytes
Desc: not available
More information about the Gcc-patches