[PATCH] make rich_location safe to copy

[PATCH] make rich_location safe to copy

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

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

[-- Attachment #2: gcc-semi_embedded_vec.diff --]
[-- Type: text/x-patch, Size: 3089 bytes --]

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.  */

Re: [PATCH] make rich_location safe to copy

[David Malcolm]

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

Re: [PATCH] make rich_location safe to copy

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

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
> 


[-- Attachment #2: error.diff --]
[-- Type: text/x-patch, Size: 3381 bytes --]

/src/gcc/master/gcc/cp/parser.c: In function ‘tree_node* cp_parser_selection_statement(cp_parser*, bool*, vec<tree_node*>*)’:
/src/gcc/master/gcc/cp/parser.c:12358:39: error: use of deleted function ‘gcc_rich_location::gcc_rich_location(gcc_rich_location&&)’
      gcc_rich_location richloc = tok->location;
                                       ^~~~~~~~
In file included from /src/gcc/master/gcc/cp/parser.c:44:
/src/gcc/master/gcc/gcc-rich-location.h:25:7: note: ‘gcc_rich_location::gcc_rich_location(gcc_rich_location&&)’ is implicitly deleted because the default definition would be ill-formed:
 class gcc_rich_location : public rich_location
       ^~~~~~~~~~~~~~~~~
/src/gcc/master/gcc/gcc-rich-location.h:25:7: error: use of deleted function ‘rich_location::rich_location(rich_location&)’
In file included from /src/gcc/master/gcc/input.h:24,
                 from /src/gcc/master/gcc/coretypes.h:482,
                 from /src/gcc/master/gcc/cp/parser.c:24:
/src/gcc/master/gcc/../libcpp/include/line-map.h:1664:7: note: ‘rich_location::rich_location(rich_location&)’ is implicitly deleted because the default definition would be ill-formed:
 class rich_location
       ^~~~~~~~~~~~~
/src/gcc/master/gcc/../libcpp/include/line-map.h:1664:7: error: use of deleted function ‘semi_embedded_vec<T, NUM_EMBEDDED>::semi_embedded_vec(semi_embedded_vec<T, NUM_EMBEDDED>&) [with T = location_range; int NUM_EMBEDDED = 3]’
/src/gcc/master/gcc/../libcpp/include/line-map.h:1379:3: note: declared here
   semi_embedded_vec (semi_embedded_vec &) = delete;
   ^~~~~~~~~~~~~~~~~
/src/gcc/master/gcc/../libcpp/include/line-map.h:1664:7: error: use of deleted function ‘semi_embedded_vec<T, NUM_EMBEDDED>::semi_embedded_vec(semi_embedded_vec<T, NUM_EMBEDDED>&) [with T = fixit_hint*; int NUM_EMBEDDED = 2]’
 class rich_location
       ^~~~~~~~~~~~~
/src/gcc/master/gcc/../libcpp/include/line-map.h:1379:3: note: declared here
   semi_embedded_vec (semi_embedded_vec &) = delete;
   ^~~~~~~~~~~~~~~~~
In file included from /src/gcc/master/gcc/cp/parser.c:44:
/src/gcc/master/gcc/gcc-rich-location.h:31:3: note:   after user-defined conversion: ‘gcc_rich_location::gcc_rich_location(location_t, const range_label*)’
   gcc_rich_location (location_t loc, const range_label *label = NULL)
   ^~~~~~~~~~~~~~~~~
/src/gcc/master/gcc/cp/parser.c:12386:46: error: use of deleted function ‘gcc_rich_location::gcc_rich_location(gcc_rich_location&&)’
   gcc_rich_location else_richloc = else_tok->location;
                                              ^~~~~~~~
In file included from /src/gcc/master/gcc/cp/parser.c:44:
/src/gcc/master/gcc/gcc-rich-location.h:31:3: note:   after user-defined conversion: ‘gcc_rich_location::gcc_rich_location(location_t, const range_label*)’
   gcc_rich_location (location_t loc, const range_label *label = NULL)
   ^~~~~~~~~~~~~~~~~


diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 7d964172469..5bedb5708ca 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1376,6 +1376,9 @@ class semi_embedded_vec
   semi_embedded_vec ();
   ~semi_embedded_vec ();
 
+  semi_embedded_vec (semi_embedded_vec &) = delete;
+  void operator= (semi_embedded_vec &) = delete;
+
   unsigned int count () const { return m_num; }
   T& operator[] (int idx);
   const T& operator[] (int idx) const;

Re: [PATCH] make rich_location safe to copy

[David Malcolm]

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

Re: [PATCH] make rich_location safe to copy

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 3164 bytes --]

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


[-- Attachment #2: gcc-rich-location.diff --]
[-- Type: text/x-patch, Size: 2626 bytes --]

gcc/cp/ChangeLog:

	* parser.c (cp_parser_selection_statement): Use direct initialization
	instead of copy.

gcc/ChangeLog:

	* gcc-rich-location.h (gcc_rich_location): Make ctor explicit.

libcpp/ChangeLog:

	* include/line-map.h (class rich_location): Disable copying and
	assignment.

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index d57ddc4560d..848e4823258 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -12355,7 +12355,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p,
 	    IF_STMT_CONSTEVAL_P (statement) = true;
 	    condition = finish_if_stmt_cond (boolean_false_node, statement);
 
-	    gcc_rich_location richloc = tok->location;
+	    gcc_rich_location richloc (tok->location);
 	    bool non_compound_stmt_p = false;
 	    if (cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE))
 	      {
@@ -12383,7 +12383,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p,
 						RID_ELSE))
 	      {
 		cp_token *else_tok = cp_lexer_peek_token (parser->lexer);
-		gcc_rich_location else_richloc = else_tok->location;
+		gcc_rich_location else_richloc (else_tok->location);
 		guard_tinfo = get_token_indent_info (else_tok);
 		/* Consume the `else' keyword.  */
 		cp_lexer_consume_token (parser->lexer);
diff --git a/gcc/gcc-rich-location.h b/gcc/gcc-rich-location.h
index 00747631025..2a9e5db65d5 100644
--- a/gcc/gcc-rich-location.h
+++ b/gcc/gcc-rich-location.h
@@ -21,14 +21,16 @@ along with GCC; see the file COPYING3.  If not see
 #define GCC_RICH_LOCATION_H
 
 /* A gcc_rich_location is libcpp's rich_location with additional
-   helper methods for working with gcc's types.  */
+   helper methods for working with gcc's types.  The class is not
+   copyable or assignable because rich_location isn't. */
+
 class gcc_rich_location : public rich_location
 {
  public:
   /* Constructors.  */
 
   /* Constructing from a location.  */
-  gcc_rich_location (location_t loc, const range_label *label = NULL)
+  explicit gcc_rich_location (location_t loc, const range_label *label = NULL)
   : rich_location (line_table, loc, label)
   {
   }
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 7d964172469..28fb2bb162b 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1670,6 +1670,10 @@ class rich_location
   /* Destructor.  */
   ~rich_location ();
 
+  /* Not copyable or assignable.  */
+  rich_location (const rich_location &) = delete;
+  void operator= (const rich_location &) = delete;
+
   /* Accessors.  */
   location_t get_loc () const { return get_loc (0); }
   location_t get_loc (unsigned int idx) const;

Re: [PATCH] make rich_location safe to copy

[Jason Merrill]

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

Re: [PATCH] make rich_location safe to copy

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

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

[-- Attachment #2: gcc-rich-location.diff --]
[-- Type: text/x-patch, Size: 2737 bytes --]

gcc/cp/ChangeLog:

	* parser.c (cp_parser_selection_statement): Use direct initialization
	instead of copy.

gcc/ChangeLog:

	* gcc-rich-location.h (gcc_rich_location): Make ctor explicit.

libcpp/ChangeLog:

	* include/line-map.h (class rich_location): Disable copying and
	assignment.

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index d57ddc4560d..848e4823258 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -12355,7 +12355,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p,
 	    IF_STMT_CONSTEVAL_P (statement) = true;
 	    condition = finish_if_stmt_cond (boolean_false_node, statement);
 
-	    gcc_rich_location richloc = tok->location;
+	    gcc_rich_location richloc (tok->location);
 	    bool non_compound_stmt_p = false;
 	    if (cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE))
 	      {
@@ -12383,7 +12383,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p,
 						RID_ELSE))
 	      {
 		cp_token *else_tok = cp_lexer_peek_token (parser->lexer);
-		gcc_rich_location else_richloc = else_tok->location;
+		gcc_rich_location else_richloc (else_tok->location);
 		guard_tinfo = get_token_indent_info (else_tok);
 		/* Consume the `else' keyword.  */
 		cp_lexer_consume_token (parser->lexer);
diff --git a/gcc/gcc-rich-location.h b/gcc/gcc-rich-location.h
index 00747631025..2a9e5db65d5 100644
--- a/gcc/gcc-rich-location.h
+++ b/gcc/gcc-rich-location.h
@@ -21,14 +21,16 @@ along with GCC; see the file COPYING3.  If not see
 #define GCC_RICH_LOCATION_H
 
 /* A gcc_rich_location is libcpp's rich_location with additional
-   helper methods for working with gcc's types.  */
+   helper methods for working with gcc's types.  The class is not
+   copyable or assignable because rich_location isn't. */
+
 class gcc_rich_location : public rich_location
 {
  public:
   /* Constructors.  */
 
   /* Constructing from a location.  */
-  gcc_rich_location (location_t loc, const range_label *label = NULL)
+  explicit gcc_rich_location (location_t loc, const range_label *label = NULL)
   : rich_location (line_table, loc, label)
   {
   }
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 7d964172469..464494bfb5b 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1670,6 +1670,12 @@ class rich_location
   /* Destructor.  */
   ~rich_location ();
 
+  /* The class manages the memory pointed to by the elements of
+     the M_FIXIT_HINTS vector and is not meant to be copied or
+     assigned.  */
+  rich_location (const rich_location &) = delete;
+  void operator= (const rich_location &) = delete;
+
   /* Accessors.  */
   location_t get_loc () const { return get_loc (0); }
   location_t get_loc (unsigned int idx) const;

Re: [PATCH] make rich_location safe to copy

[David Malcolm]

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][PATCH] make rich_location safe to copy

[Martin Sebor]

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