* Re: [pph] Namespaces, step 1. Trace formatting. (issue4433054)
@ 2011-04-20 15:22 dnovillo
2011-04-20 17:59 ` Lawrence Crowl
0 siblings, 1 reply; 3+ messages in thread
From: dnovillo @ 2011-04-20 15:22 UTC (permalink / raw)
To: crowl; +Cc: gcc-patches, reply
http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.c
File gcc/cp/pph-streamer.c (right):
http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.c#newcode144
gcc/cp/pph-streamer.c:144: return;
+ if ((type == PPH_TRACE_TREE || type == PPH_TRACE_CHAIN)
+ && !data && flag_pph_tracer <= 3)
+ return;
Line up the predicates vertically.
http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.c#newcode172
gcc/cp/pph-streamer.c:172: fprintf (pph_logfile, ", code=%s",
tree_code_name[TREE_CODE (t)]);
case PPH_TRACE_REF:
+ {
+ const_tree t = (const_tree) data;
+ if (t)
+ {
+ print_generic_expr (pph_logfile, CONST_CAST (union tree_node *,
t),
+ 0);
+ fprintf (pph_logfile, ", code=%s", tree_code_name[TREE_CODE (t)]);
But how are we going to tell if this is a REF instead of a tree? The
output seems identical to the PPH_TRACE_TREE case.
http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.h
File gcc/cp/pph-streamer.h (right):
http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.h#newcode149
gcc/cp/pph-streamer.h:149: }
pph_output_tree_lst (pph_stream *stream, tree t, bool ref_p)
+{
+ if (flag_pph_tracer >= 2)
+ pph_stream_trace_tree (stream, t, ref_p);
+ lto_output_tree (stream->ob, t, ref_p);
+}
I don't really like all this code duplication. Wouldn't it be better if
instead of having pph_output_tree_aux and pph_output_tree_lst, we added
another argument to pph_output_tree? The argument would be an enum and
we could have a default 'DONT_CARE' value.
http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.h#newcode298
gcc/cp/pph-streamer.h:298: pph_stream_trace_tree (stream, t, false); /*
FIXME pph: always false? */
@@ -285,7 +295,7 @@ pph_input_tree (pph_stream *stream)
{
tree t = lto_input_tree (stream->ib, stream->data_in);
if (flag_pph_tracer >= 4)
- pph_stream_trace_tree (stream, t);
+ pph_stream_trace_tree (stream, t, false); /* FIXME pph: always
false?
Yes, on input we can't tell if we read a reference or a real tree. We
could, but not at this level. That's inside the actual LTO streaming
code.
http://codereview.appspot.com/4433054/
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pph] Namespaces, step 1. Trace formatting. (issue4433054)
2011-04-20 15:22 [pph] Namespaces, step 1. Trace formatting. (issue4433054) dnovillo
@ 2011-04-20 17:59 ` Lawrence Crowl
0 siblings, 0 replies; 3+ messages in thread
From: Lawrence Crowl @ 2011-04-20 17:59 UTC (permalink / raw)
To: crowl, dnovillo, gcc-patches, reply
On 4/20/11, dnovillo@google.com <dnovillo@google.com> wrote:
> http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.c
> File gcc/cp/pph-streamer.c (right):
>
> http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.c#newcode144
> gcc/cp/pph-streamer.c:144: return;
> + if ((type == PPH_TRACE_TREE || type == PPH_TRACE_CHAIN)
> + && !data && flag_pph_tracer <= 3)
> + return;
>
> Line up the predicates vertically.
Can you be more specific?
>
> http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.c#newcode172
> gcc/cp/pph-streamer.c:172: fprintf (pph_logfile, ", code=%s",
> tree_code_name[TREE_CODE (t)]);
> case PPH_TRACE_REF:
> + {
> + const_tree t = (const_tree) data;
> + if (t)
> + {
> + print_generic_expr (pph_logfile, CONST_CAST (union tree_node *,
> t),
> + 0);
> + fprintf (pph_logfile, ", code=%s", tree_code_name[TREE_CODE (t)]);
>
>
> But how are we going to tell if this is a REF instead of a tree?
The type_s array is indexed by PPH_TRACE_REF.
> The output seems identical to the PPH_TRACE_TREE case.
Well, the case in those branches is identical. The splitting was
a bit preemptive, as I was planning to see what changes I needed
after seeing what items were refs. None actually were refs, so
the distinction isn't there.
> http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.h
> File gcc/cp/pph-streamer.h (right):
>
> http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.h#newcode149
> gcc/cp/pph-streamer.h:149: }
> pph_output_tree_lst (pph_stream *stream, tree t, bool ref_p)
> +{
> + if (flag_pph_tracer >= 2)
> + pph_stream_trace_tree (stream, t, ref_p);
> + lto_output_tree (stream->ob, t, ref_p);
> +}
>
> I don't really like all this code duplication. Wouldn't it be better if
> instead of having pph_output_tree_aux and pph_output_tree_lst, we added
> another argument to pph_output_tree? The argument would be an enum and
> we could have a default 'DONT_CARE' value.
I'm not sure that would save much code. It would induce some
runtime overhead (unless the compiler specialized routines).
It would also change the callbacks.
> http://codereview.appspot.com/4433054/diff/1/gcc/cp/pph-streamer.h#newcode298
> gcc/cp/pph-streamer.h:298: pph_stream_trace_tree (stream, t, false); /*
> FIXME pph: always false? */
> @@ -285,7 +295,7 @@ pph_input_tree (pph_stream *stream)
> {
> tree t = lto_input_tree (stream->ib, stream->data_in);
> if (flag_pph_tracer >= 4)
> - pph_stream_trace_tree (stream, t);
> + pph_stream_trace_tree (stream, t, false); /* FIXME pph: always
> false?
>
> Yes, on input we can't tell if we read a reference or a real tree. We
> could, but not at this level. That's inside the actual LTO streaming
> code.
It would be nice to have an indication, but it is not something I want
to do now.
>
> http://codereview.appspot.com/4433054/
--
Lawrence Crowl
^ permalink raw reply [flat|nested] 3+ messages in thread
* [pph] Namespaces, step 1. Trace formatting. (issue4433054)
@ 2011-04-19 2:13 Lawrence Crowl
0 siblings, 0 replies; 3+ messages in thread
From: Lawrence Crowl @ 2011-04-19 2:13 UTC (permalink / raw)
To: reply, dnovillo, gcc-patches
First stab at getting namespaces working with PPH. This change will
put top-level namespaces into the global namespace. It does not,
however, appear to their members in any place useful.
Some tuning of tracing output.
Index: gcc/cp/ChangeLog.pph
2011-04-18 Lawrence Crowl <crowl@google.com>
* pph.c (pth_save_identifiers): Fix FIXME comment.
(pph_add_names_to_namespace): Add contained namespaces to scope as
well as regular symbols.
* pph-streamer.c (enum pph_trace_type): Add a trace for refs.
(pph_stream_trace): Add the ref printing. Reduce bulk of trace
messages. Filter off uninteresting traces, primarily traces of null
pointers.
(pph_stream_trace_tree): Convert ref_p to trace value.
* pph-streamer.h: Adjust flag_pph_tracer comparisons for better traces.
(pph_stream_trace_tree): Add ref_p parameter. Propogate such
parameters at all call sites.
(pph_output_tree_lst): New. Like pph_output_tree, but for list and
chain contexts.
(pph_stream_write_tree): The callback is auxillary.
(pph_output_chain_filtered): Call pph_output_tree_lst.
Index: libcpp/ChangeLog.pph
2011-04-18 Lawrence Crowl <crowl@google.com>
* init.c: Fix spelling in comment.
Index: gcc/cp/pph.c
===================================================================
--- gcc/cp/pph.c (revision 172514)
+++ gcc/cp/pph.c (working copy)
@@ -792,7 +792,7 @@ pth_save_identifiers (cpp_idents_used *i
if (!(entry->used_by_directive || entry->expanded_to_text))
continue;
- /* FIX pph: We are wasting space; ident_len, used_by_directive
+ /* FIXME pph: We are wasting space; ident_len, used_by_directive
and expanded_to_text together could fit into a single uint. */
pph_output_uint (stream, entry->used_by_directive);
@@ -1956,6 +1956,21 @@ pph_add_names_to_namespace (tree ns, tre
chain = DECL_CHAIN (t);
pushdecl_with_scope (t, level, /*is_friend=*/false);
}
+
+ for (t = new_level->namespaces; t; t = chain)
+ {
+ /* Pushing a decl into a scope clobbers its DECL_CHAIN.
+ Preserve it. */
+ /* FIXME pph: we should first check to see if it isn't already there. */
+ chain = DECL_CHAIN (t);
+ pushdecl_with_scope (t, level, /*is_friend=*/false);
+ /* FIXME pph: The change above enables the namespace,
+ but its symbols are still missing.
+ The recursive call below causes multiple errors.
+ pph_add_names_to_namespace (t, t);
+ */
+ }
+
}
Index: gcc/cp/pph-streamer.c
===================================================================
--- gcc/cp/pph-streamer.c (revision 172514)
+++ gcc/cp/pph-streamer.c (working copy)
@@ -118,6 +118,7 @@ pph_stream_close (pph_stream *stream)
enum pph_trace_type
{
PPH_TRACE_TREE,
+ PPH_TRACE_REF,
PPH_TRACE_UINT,
PPH_TRACE_BYTES,
PPH_TRACE_STRING,
@@ -134,11 +135,15 @@ static void
pph_stream_trace (pph_stream *stream, const void *data, unsigned int nbytes,
enum pph_trace_type type)
{
- const char *op = (stream->write_p) ? "write" : "read";
- const char *type_s[] = { "tree", "uint", "bytes", "string", "chain",
+ const char *op = (stream->write_p) ? "<<" : ">>";
+ const char *type_s[] = { "tree", "ref", "uint", "bytes", "string", "chain",
"bitpack" };
- fprintf (pph_logfile, "*** %s: op=%s, type=%s, size=%u, value=",
+ if ((type == PPH_TRACE_TREE || type == PPH_TRACE_CHAIN)
+ && !data && flag_pph_tracer <= 3)
+ return;
+
+ fprintf (pph_logfile, "*** %s: %s%s/%u, value=",
stream->name, op, type_s[type], (unsigned) nbytes);
switch (type)
@@ -157,6 +162,20 @@ pph_stream_trace (pph_stream *stream, co
}
break;
+ case PPH_TRACE_REF:
+ {
+ const_tree t = (const_tree) data;
+ if (t)
+ {
+ print_generic_expr (pph_logfile, CONST_CAST (union tree_node *, t),
+ 0);
+ fprintf (pph_logfile, ", code=%s", tree_code_name[TREE_CODE (t)]);
+ }
+ else
+ fprintf (pph_logfile, "NULL_TREE");
+ }
+ break;
+
case PPH_TRACE_UINT:
{
unsigned int val = *((const unsigned int *) data);
@@ -212,10 +231,10 @@ pph_stream_trace (pph_stream *stream, co
/* Show tracing information for T on STREAM. */
void
-pph_stream_trace_tree (pph_stream *stream, tree t)
+pph_stream_trace_tree (pph_stream *stream, tree t, bool ref_p)
{
pph_stream_trace (stream, t, t ? tree_code_size (TREE_CODE (t)) : 0,
- PPH_TRACE_TREE);
+ ref_p ? PPH_TRACE_REF : PPH_TRACE_TREE);
}
Index: gcc/cp/pph-streamer.h
===================================================================
--- gcc/cp/pph-streamer.h (revision 172514)
+++ gcc/cp/pph-streamer.h (working copy)
@@ -93,7 +93,7 @@ enum chain_filter { NONE, NO_BUILTINS };
/* In pph-streamer.c. */
pph_stream *pph_stream_open (const char *, const char *);
void pph_stream_close (pph_stream *);
-void pph_stream_trace_tree (pph_stream *, tree);
+void pph_stream_trace_tree (pph_stream *, tree, bool ref_p);
void pph_stream_trace_uint (pph_stream *, unsigned int);
void pph_stream_trace_bytes (pph_stream *, const void *, size_t);
void pph_stream_trace_string (pph_stream *, const char *);
@@ -134,17 +134,27 @@ static inline void
pph_output_tree (pph_stream *stream, tree t, bool ref_p)
{
if (flag_pph_tracer >= 1)
- pph_stream_trace_tree (stream, t);
+ pph_stream_trace_tree (stream, t, ref_p);
lto_output_tree (stream->ob, t, ref_p);
}
/* Output AST T to STREAM. If REF_P is true, output all the leaves of T
- as references. this function is an internal auxillary routine. */
+ as references. This function is an list auxillary routine. */
+static inline void
+pph_output_tree_lst (pph_stream *stream, tree t, bool ref_p)
+{
+ if (flag_pph_tracer >= 2)
+ pph_stream_trace_tree (stream, t, ref_p);
+ lto_output_tree (stream->ob, t, ref_p);
+}
+
+/* Output AST T to STREAM. If REF_P is true, output all the leaves of T
+ as references. This function is an internal auxillary routine. */
static inline void
pph_output_tree_aux (pph_stream *stream, tree t, bool ref_p)
{
if (flag_pph_tracer >= 3)
- pph_stream_trace_tree (stream, t);
+ pph_stream_trace_tree (stream, t, ref_p);
lto_output_tree (stream->ob, t, ref_p);
}
@@ -153,7 +163,7 @@ static inline void
pph_output_tree_or_ref (pph_stream *stream, tree t, bool ref_p)
{
if (flag_pph_tracer >= 2)
- pph_stream_trace_tree (stream, t);
+ pph_stream_trace_tree (stream, t, ref_p);
lto_output_tree_or_ref (stream->ob, t, ref_p);
}
@@ -220,7 +230,7 @@ pph_output_string_with_length (pph_strea
static inline void
pph_output_chain (pph_stream *stream, tree first, bool ref_p)
{
- if (flag_pph_tracer >= 3)
+ if (flag_pph_tracer >= 2)
pph_stream_trace_chain (stream, first);
lto_output_chain (stream->ob, first, ref_p);
}
@@ -285,7 +295,7 @@ pph_input_tree (pph_stream *stream)
{
tree t = lto_input_tree (stream->ib, stream->data_in);
if (flag_pph_tracer >= 4)
- pph_stream_trace_tree (stream, t);
+ pph_stream_trace_tree (stream, t, false); /* FIXME pph: always false? */
return t;
}
Index: gcc/cp/pph-streamer-out.c
===================================================================
--- gcc/cp/pph-streamer-out.c (revision 172514)
+++ gcc/cp/pph-streamer-out.c (working copy)
@@ -791,7 +791,7 @@ pph_stream_write_tree (struct output_blo
}
if (TREE_CODE (expr) == TYPE_DECL)
- pph_output_tree (stream, DECL_ORIGINAL_TYPE (expr), ref_p);
+ pph_output_tree_aux (stream, DECL_ORIGINAL_TYPE (expr), ref_p);
}
else if (TREE_CODE (expr) == STATEMENT_LIST)
{
@@ -855,7 +855,7 @@ pph_output_chain_filtered (pph_stream *s
saved_chain = TREE_CHAIN (t);
TREE_CHAIN (t) = NULL_TREE;
- pph_output_tree (stream, t, ref_p);
+ pph_output_tree_lst (stream, t, ref_p);
TREE_CHAIN (t) = saved_chain;
}
Index: libcpp/init.c
===================================================================
--- libcpp/init.c (revision 172514)
+++ libcpp/init.c (working copy)
@@ -395,7 +395,7 @@ static const struct builtin_operator ope
};
#undef B
-/* Verify that the indicies of the named operators fit within the
+/* Verify that the indices of the named operators fit within the
number of bits available. */
#define B(n, t) unsigned char t ## _too_large_for_bitfield[ \
--
This patch is available for review at http://codereview.appspot.com/4433054
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-04-20 17:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-20 15:22 [pph] Namespaces, step 1. Trace formatting. (issue4433054) dnovillo
2011-04-20 17:59 ` Lawrence Crowl
-- strict thread matches above, loose matches on Subject: below --
2011-04-19 2:13 Lawrence Crowl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).