diff mbox

[3.1/11] Explicitly initialize the macro-generated pass fields (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)

Message ID 1375490332.4994.79.camel@surprise
State New
Headers show

Commit Message

David Malcolm Aug. 3, 2013, 12:38 a.m. UTC 
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)

Comments

Richard Henderson Aug. 3, 2013, 5:59 p.m. UTC | #1 
On 08/02/2013 02:38 PM, David Malcolm wrote:
> OK for trunk? (as part of patches 3-6)

Ok.
diff mbox

Patch

From 3198835b85b9a35906bf93ba8f510762e8e017b9 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@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