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