On Thu, Aug 29, 2024 at 06:58:12PM -0400, David Malcolm wrote:
> The following patch rewrites the internals of pp_format.
> The tokens and token lists are allocated on the chunk_obstack, and so
> there's no additional heap activity required, with the memory reclaimed
> when the chunk_obstack is freed after phase 3 of formatting.
> +static void *
> +allocate_object (size_t sz, obstack &s)
> +{
> + /* We must not be half-way through an object. */
> + gcc_assert (obstack_base (&s) == obstack_next_free (&s));
> +
> + obstack_grow (&s, obstack_base (&s), sz);
> + void *buf = obstack_finish (&s);
> + return buf;
> }
I think this is wrong. I hoped it would be the reason of the
unexpected libstdc++ warnings on certain architectures after
seeing
==4027220== Source and destination overlap in memcpy(0x4627154, 0x4627154, 12)
==4027220== at 0x404B93E: memcpy (vg_replace_strmem.c:1123)
==4027220== by 0xAAD5618: allocate_object(unsigned int, obstack&)
(pretty-print.cc:1183)
==4027220== by 0xAAD8C0E: operator new (pretty-print.cc:1210)
==4027220== by 0xAAD8C0E: make (pretty-print-format-impl.h:305)
==4027220== by 0xAAD8C0E: format_phase_1 (pretty-print.cc:1659)
==4027220== by 0xAAD8C0E: pretty_printer::format(text_info&)
(pretty-print.cc:1618)
==4027220== by 0xAAA840E: pp_format (pretty-print.h:583)
==4027220== by 0xAAA840E:
diagnostic_context::report_diagnostic(diagnostic_info*) (diagnostic.cc:1260)
==4027220== by 0xAAA8703:
diagnostic_context::diagnostic_impl(rich_location*, diagnostic_metadata const*,
diagnostic_option_id, char const*, char**, diagnostic_t) (diagnostic.cc:1404)
==4027220== by 0xAAB8682: warning(diagnostic_option_id, char const*, ...)
(diagnostic-global-context.cc:166)
==4027220== by 0x97725F5: warn_deprecated_use(tree_node*, tree_node*)
(tree.cc:12485)
==4027220== by 0x8B6694B: mark_used(tree_node*, int) (decl2.cc:6121)
==4027220== by 0x8C9E25E: tsubst_expr(tree_node*, tree_node*, int,
tree_node*) [clone .part.0] (pt.cc:21626)
==4027220== by 0x8C9E5E6: tsubst_expr(tree_node*, tree_node*, int,
tree_node*) [clone .part.0] (pt.cc:20935)
==4027220== by 0x8C9E1D7: tsubst_expr(tree_node*, tree_node*, int,
tree_node*) [clone .part.0] (pt.cc:20424)
==4027220== by 0x8C9DF2E: tsubst_expr(tree_node*, tree_node*, int,
tree_node*) [clone .part.0] (pt.cc:20496)
==4027220==
etc. valgrind warnings, unfortunately it is not, but still
I think this is a bug.
If the obstack has enough space in it, i.e. if obstack_room (&s) >= sz,
then obstack_grow from obstack_base will copy uninitialized bytes
through memcpy (obstack_base (&s), obstack_base (&s), sz);
(which pedantically isn't valid due to the overlap, and so
the reason why valgrind complains, but in reality I think most
implementations can handle it fine, after all, we also use it for
structure assignments which could have full or no overlap but never
partial).
If obstack_room (&s) < sz, then obstack_grow will first
_obstack_newchunk (&s, sz); which will allocate new memory and
copy the existing data of the object (but the above assertion
guartantees it will copy 0 bytes) and then the memcpy copies
sz bytes from the old base to the new (if unlucky, that could crash
as there could be end of page and unmapped next page in between).
I think we should use obstack_blank instead of obstack_grow, which
does everything obstack_grow does, except for the memcpy of the
uninitialized data.
Bootstrapped/regtested on i686-linux, ok for trunk?
2024-09-25 Jakub Jelinek <ja...@redhat.com>
* pretty-print.cc (allocate_object): Use obstack_blank rather than
obstack_grow.
--- gcc/pretty-print.cc.jj 2024-09-24 15:14:54.116162039 +0200
+++ gcc/pretty-print.cc 2024-09-25 22:12:57.776029725 +0200
@@ -1180,7 +1180,7 @@ allocate_object (size_t sz, obstack &s)
/* We must not be half-way through an object. */
gcc_assert (obstack_base (&s) == obstack_next_free (&s));
- obstack_grow (&s, obstack_base (&s), sz);
+ obstack_blank (&s, sz);
void *buf = obstack_finish (&s);
return buf;
}
Jakub
