On Thu, 26 Sep 2024, Jakub Jelinek wrote:
> 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?
OK.
Thanks,
Richard.
> 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
>
>
