| Message ID | 88448f63-87ad-c3a5-d38b-c94dd825e8d2@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | make rich_location safe to copy | expand |
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. > > 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? > > Tested on x86_64-linux. > > Martin Thanks Dave
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. 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 >
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 > > >
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 >>> >> > >
On 6/16/21 12:11 PM, Martin Sebor wrote: > 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 C++ bits are fine. > 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). > + /* Not copyable or assignable. */ This comment needs a rationale. Jason
On 6/16/21 10:35 AM, Jason Merrill wrote: > On 6/16/21 12:11 PM, Martin Sebor wrote: >> 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 C++ bits are fine. > >> 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). > >> + /* Not copyable or assignable. */ > > This comment needs a rationale. Done in the attached patch. Providing a rationale in each instance sounds like a good addition to the coding conventions. Let me propose a patch for that. Martin
On Wed, 2021-06-16 at 11:21 -0600, Martin Sebor wrote: > On 6/16/21 10:35 AM, Jason Merrill wrote: > > On 6/16/21 12:11 PM, Martin Sebor wrote: > > > 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 C++ bits are fine. > > > > > 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). > > > > > + /* Not copyable or assignable. */ > > > > This comment needs a rationale. > > Done in the attached patch. LGTM; thanks Dave > > Providing a rationale in each instance sounds like a good addition > to the coding conventions. Let me propose a patch for that. > > Martin
Ping: David, I'm still looking for approval of the semi_embedded_vec change in the originally posted patch (independent of the already approved subsequent change to rich_location). https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572845.html On 6/15/21 7:48 PM, Martin Sebor wrote: > 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. > > The attached patch defines the copy ctor and also copy assignment > and adds the corresponding move functions. > > Tested on x86_64-linux. > > Martin
libcpp/ChangeLog: * include/line-map.h (class semi_embedded_vec): Add copy ctor and assignment, move ctor and move assignment. diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index 7d964172469..1d6fb0d6b00 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -1376,6 +1376,12 @@ class semi_embedded_vec semi_embedded_vec (); ~semi_embedded_vec (); + semi_embedded_vec (const semi_embedded_vec &); + semi_embedded_vec (semi_embedded_vec &&); + + semi_embedded_vec& operator= (const semi_embedded_vec &); + semi_embedded_vec& operator= (semi_embedded_vec &&); + unsigned int count () const { return m_num; } T& operator[] (int idx); const T& operator[] (int idx) const; @@ -1395,8 +1401,89 @@ class semi_embedded_vec template <typename T, int NUM_EMBEDDED> semi_embedded_vec<T, NUM_EMBEDDED>::semi_embedded_vec () -: m_num (0), m_alloc (0), m_extra (NULL) + : m_num (0), m_alloc (0), m_extra (NULL) +{ +} + +/* Copy ctor. */ + +template <typename T, int NUM_EMBEDDED> +semi_embedded_vec<T, NUM_EMBEDDED>:: +semi_embedded_vec (const semi_embedded_vec &rhs) + : m_num (rhs.m_num), m_alloc (rhs.m_alloc) { + int nemb = rhs.m_num < NUM_EMBEDDED ? rhs.m_num : NUM_EMBEDDED; + memcpy (m_embedded, rhs.m_embedded, nemb * sizeof (T)); + + if (!m_extra) + { + m_extra = NULL; + return; + } + + m_extra = XNEWVEC (T, m_alloc); + memcpy (m_extra, rhs.m_extra, m_alloc * sizeof (T)); +} + +/* Move ctor. */ + +template <typename T, int NUM_EMBEDDED> +semi_embedded_vec<T, NUM_EMBEDDED>:: +semi_embedded_vec (semi_embedded_vec &&rhs) + : m_num (rhs.m_num), m_alloc (rhs.m_alloc) +{ + int nemb = rhs.m_num < NUM_EMBEDDED ? rhs.m_num : NUM_EMBEDDED; + memcpy (m_embedded, rhs.m_embedded, nemb * sizeof (T)); + + m_extra = rhs.m_extra; + rhs.m_extra = NULL; +} + +/* Copy assignment. */ + +template <typename T, int NUM_EMBEDDED> +semi_embedded_vec<T, NUM_EMBEDDED>& +semi_embedded_vec<T, NUM_EMBEDDED>::operator= (const semi_embedded_vec &rhs) +{ + int nemb = rhs.m_num < NUM_EMBEDDED ? rhs.m_num : NUM_EMBEDDED; + memcpy (m_embedded, rhs.m_embedded, nemb * sizeof (T)); + + m_num = rhs.m_num; + + if (!rhs.m_extra) + /* Don't release already allocated memory. */ + return *this; + + if (m_alloc < rhs.m_alloc) + { + m_extra = XRESIZEVEC (T, m_extra, rhs.m_alloc); + m_alloc = rhs.m_alloc; + } + + memcpy (m_extra, rhs.m_extra, m_alloc * sizeof (T)); + return *this; +} + +/* Move assignment. */ + +template <typename T, int NUM_EMBEDDED> +semi_embedded_vec<T, NUM_EMBEDDED>& +semi_embedded_vec<T, NUM_EMBEDDED>::operator= (semi_embedded_vec &&rhs) +{ + int nemb = rhs.m_num < NUM_EMBEDDED ? rhs.m_num : NUM_EMBEDDED; + memcpy (m_embedded, rhs.m_embedded, nemb * sizeof (T)); + + m_num = rhs.m_num; + + if (!rhs.m_extra) + /* Don't release already allocated memory. */ + return *this; + + m_extra = rhs.m_extra; + m_alloc = rhs.m_alloc; + + rhs.m_extra = NULL; + return *this; } /* semi_embedded_vec's dtor. Release any dynamically-allocated memory. */