On Thu, 2013-08-01 at 13:13 -0400, David Malcolm wrote: > On Mon, 2013-07-29 at 15:41 -0600, Jeff Law wrote: > > On 07/26/2013 09:04 AM, David Malcolm wrote: > > > This patch is the hand-written part of the conversion of passes from > > > C structs to C++ classes. It does not work without the subsequent > > > autogenerated part, which is huge. > > [ ... ] > > With the changes from pipeline -> pass_manager this is fine. As is the > > follow-up autogenerated patch. > > Thanks. > > The current status is that Jeff has approved patches 1-5 for the trunk, > and patches 6-11 haven't yet been reviewed by a global reviewer. I have > committed patches 1 and 2, having bootstrapped+tested each in turn, but > I can't commit any more of these without further review - details > follow. > > I had only bootstrapped and tested the combination of all 11 patches > together, so I've been attempting to test the approved patches. For > reference: > * Patch 3 is the handwritten part of the conversion of passes to C++ > classes > * Patch 4 is the autogenerated part > * Patch 5 adds -fno-rtti to the testsuite when building plugins > * Patch 6 is the not-yet-approved patch currently named "Rewrite how > instances of passes are cloned" (but that's not all it does, see below). > > I had thought that the combination of patch 3 + 4 + 5 would work as a > unit, and hoped to commit each of these after testing the combination of > (3, 4, 5), but upon attempting a bootstrap I found two bugs: > > (a) patch 3 adds an gcc_assert to the pass_manager constructor to > ensure that pass instance fields are NULL when created, to ensure that > the macro-driven logic is correct. However, the instance fields > haven't been explicitly initialized at that point, leading to sporadic > assertion failures. This wasn't an issue for the full patch series > since patch 11 adds an operator new for pass_manager, allocating it from > the GC heap, using the *cleared* allocator: > void* > pass_manager::operator new (size_t sz) > { > return ggc_internal_cleared_alloc_stat (sz MEM_STAT_INFO); > } > hence ensuring that the fields are zeroed. So patch 3 works with patch > 11, but not without. In the meantime, I'm attaching a new patch "3.1", > which explicitly zeroes these fields early on in the pass_manager ctor. > > (b) with just these patches, although static_pass_number for every > pass instance is correct, the dumpfile *switch* numbering is wrong, > leading to numerous testsuite failures (e.g. "unrecognized command line > option '-fdump-tree-dce2'") . Patch 6 is needed: it does the necessary > fixups at the right time to ensure that the per-pass-instance dump files > get the correct switch names (I'll add some more notes on this to that > email). > > With the attached patch, the combination of patches (03, 03.1, 04, 05 > *and* 06) successfully bootstraps on x86_64-unknown-linux-gnu, and all > testcases show the same results as an unpatched build (relative to > r201397). > > So I'm looking for review of the attached patch, and of at least patch 6 > before I can proceed with this. AIUI, Jeff is away at the moment, so > another global reviewer would need to do it. Here's a better version of this patch: instead of explicitly initializing the fields (which would become redundant when patch 11 goes in), instead this patch adds an operator new, using xcalloc. This operator new gets reimplemented in a later version of patch 11 to use ggc_internal_cleared_alloc_stat when the class becomes GC-managed, hence at each stage of committing, the initialization is sane.
I've successfully bootstrapped the *end result* of the revised patch series 3-11 on x86_64-unknown-linux-gnu: all testcases show the same results as an unpatched build (relative to r201397). OK for trunk? (as part of patches 3-6)
>From 3198835b85b9a35906bf93ba8f510762e8e017b9 Mon Sep 17 00:00:00 2001 From: David Malcolm <dmalc...@redhat.com> Date: Wed, 31 Jul 2013 14:20:07 -0400 Subject: [PATCH 04/15] Zero-initialize pass_manager Ensure that pass_manager instances are fully zero-initialized, by providing an operator new, implemented using xcalloc. gcc/ * passes.c (pass_manager::operator new): New. --- gcc/pass_manager.h | 2 ++ gcc/passes.c | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h index ea078a5..00f0b1c 100644 --- a/gcc/pass_manager.h +++ b/gcc/pass_manager.h @@ -47,6 +47,8 @@ class context; class pass_manager { public: + void *operator new (size_t sz); + pass_manager(context *ctxt); void register_pass (struct register_pass_info *pass_info); diff --git a/gcc/passes.c b/gcc/passes.c index fcbd630..8efce30 100644 --- a/gcc/passes.c +++ b/gcc/passes.c @@ -1339,6 +1339,13 @@ pass_manager::register_pass (struct register_pass_info *pass_info) -> all_passes */ +void * +pass_manager::operator new (size_t sz) +{ + /* Ensure that all fields of the pass manager are zero-initialized. */ + return xcalloc (1, sz); +} + pass_manager::pass_manager (context *ctxt) : all_passes(NULL), all_small_ipa_passes(NULL), all_lowering_passes(NULL), all_regular_ipa_passes(NULL), all_lto_gen_passes(NULL), -- 1.7.11.7