[PATCH 3.1/11] Explicitly initialize the macro-generated pass fields (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)
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.
Thanks
Dave
>From 49d7df3c645459a0f90179dfb1a24cef459b186e Mon Sep 17 00:00:00 2001
From: David Malcolm
Date: Wed, 31 Jul 2013 14:20:07 -0400
Subject: [PATCH 03.1/15] Explicitly initialize the macro-generated pass fields
to NULL
gcc/
* passes.c (pass_manager::pass_manager): Explicitly initialize
the macro-generated pass fields to NULL.
---
gcc/passes.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/gcc/passes.c b/gcc/passes.c
index fcbd630..c4f4f39 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -1352,6 +1352,28 @@ pass_manager::pass_manager (context *ctxt)
GCC_PASS_LISTS
#undef DEF_PASS_LIST
+ /* For safety, NULL-initialize the various fields for individual pass
+ instances (mainly so that the:
+ gcc_assert (NULL == PASS ## _ ## NUM);
+ sees an initial NULL value).
+
+ We cannot use member initializers for this since pass-instances.def
+ contains semicolons. */
+
+#define INSERT_PASSES_AFTER(PASS)
+#define PUSH_INSERT_PASSES_WITHIN(PASS)
+#define POP_INSERT_PASSES()
+#define NEXT_PASS(PASS, NUM) PASS ## _ ## NUM = NULL;
+#define TERMINATE_PASS_LIST()
+
+#include "pass-instances.def"
+
+#undef INSERT_PASSES_AFTER
+#undef PUSH_INSERT_PASSES_WITHIN
+#undef POP_INSERT_PASSES
+#undef NEXT_PASS
+#undef TERMINATE_PASS_LIST
+
/* Build the tree of passes. */
#define INSERT_PASSES_AFTER(PASS) \
--
1.7.11.7
Re: [PATCH 3.1/11] Explicitly initialize the macro-generated pass fields (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)
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
Re: [PATCH 3.1/11] Explicitly initialize the macro-generated pass fields (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)
On 08/02/2013 02:38 PM, David Malcolm wrote: > OK for trunk? (as part of patches 3-6) Ok.
