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