Re: [pph] Clean positive tests. (issue4423044)

[dnovillo]

On 2011/04/15 18:22:19, Lawrence Crowl wrote:


2011-04-15  Lawrence Crowl  



* lib/dg-pph.exp (dg-pph-pos): Stop on first failure.  Change names

of

assembly files to be clear on which is with and without PPH.


OK.


Diego.

http://codereview.appspot.com/4423044/

Re: [pph] Namespaces, step 1. Trace formatting. (issue4433054)

[dnovillo]

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/

Re: [google] Use R_ARM_GOT_PREL to simplify global address loading from GOT (issue4433079)

[dnovillo]

I only have some stylistic comments for this patch.  The new pass looks
OK to me, but I do not know this area well enough to do a good review.

In your ChangeLog entries, please remove the directory prefix from the
file names.


http://codereview.appspot.com/4433079/diff/1/gcc/hooks.c
File gcc/hooks.c (right):

http://codereview.appspot.com/4433079/diff/1/gcc/hooks.c#newcode287
gcc/hooks.c:287: return NULL;
+hook_rtx_void_null (void)
+{
+  return NULL;

s/NULL/NULL_RTX/

http://codereview.appspot.com/4433079/diff/1/gcc/simplify-got.c
File gcc/simplify-got.c (right):

http://codereview.appspot.com/4433079/diff/1/gcc/simplify-got.c#newcode83
gcc/simplify-got.c:83: return (optimize > 0) &&
targetm.got_access.get_pic_reg ();
+{
+  return (optimize > 0) && targetm.got_access.get_pic_reg ();

s/(optimize > 0)/optimize/

http://codereview.appspot.com/4433079/diff/1/gcc/simplify-got.c#newcode118
gcc/simplify-got.c:118: if (!(set && (SET_DEST (set) == pic_reg)))
+ /* If an insn both set and use pic_reg, it is in the process of
+constructing the value of pic_reg. We should also ignore it.  */
+ rtx set = single_set (insn);
+ if (!(set && (SET_DEST (set) == pic_reg)))

Extra ( ) around SET_DEST are not needed.

http://codereview.appspot.com/4433079/

Re: [google] Check if the nonnull attribute is applied to 'this' (issue4446070)

[dnovillo]

On 2011/04/29 15:12:52, richard.guenther_gmail.com wrote:


>
> +



spurious white-space change.


Thanks.  Fixed.


Diego.

http://codereview.appspot.com/4446070/

Re: [google] Use different peeling parameters with available profile (issue4438079)

[dnovillo]

On 2011/04/29 00:02:40, singhai wrote:


2011-04-28  Sharad Singhai  



gcc/ChangeLog.google-main
* params.def: Add new parameters to control peeling.
* tree-ssa-loop-ivcanon.c (try_unroll_loop_completely): Use
different peeling parameters when profile feedback is available.
* loop-unroll.c (decide_peel_once_rolling): Ditto.
(decide_peel_completely): Ditto.
* doc/invoke.texi: Document new peeling parameters.



testsuite/ChangeLog.google-main
* gcc.dg/vect/O3-vect-pr34223.c: Add extra peeling
parameters.
* gcc.dg/vect/vect.exp: Allow reading flags in individual
tests.


OK for google/main.


Diego.

http://codereview.appspot.com/4438079/

Re: Disable tracer by default for profile use (issue4428074)

[dnovillo]

On 2011/04/28 18:53:24, Diego Novillo wrote:


OK.


Committed rev 173186.


Diego.

http://codereview.appspot.com/4428074/

Re: [google] Use different peeling parameters with available profile (issue4438079)

[dnovillo]

On 2011/04/29 19:21:00, Diego Novillo wrote:

On 2011/04/29 00:02:40, singhai wrote:



> 2011-04-28  Sharad Singhai  
>
>gcc/ChangeLog.google-main
>* params.def: Add new parameters to control peeling.
>* tree-ssa-loop-ivcanon.c (try_unroll_loop_completely): Use
>different peeling parameters when profile feedback is available.
>* loop-unroll.c (decide_peel_once_rolling): Ditto.
>(decide_peel_completely): Ditto.
>* doc/invoke.texi: Document new peeling parameters.
>
>testsuite/ChangeLog.google-main
>* gcc.dg/vect/O3-vect-pr34223.c: Add extra peeling
>parameters.
>* gcc.dg/vect/vect.exp: Allow reading flags in individual
>tests.



OK for google/main.



Committed rev 173187.


Diego.

http://codereview.appspot.com/4438079/

Re: [pph] Enable nested namespaces (issue4431071)

[dnovillo]

Looks OK.  Some comments below.


http://codereview.appspot.com/4431071/diff/1/gcc/c-family/c.opt
File gcc/c-family/c.opt (right):

http://codereview.appspot.com/4431071/diff/1/gcc/c-family/c.opt#newcode943
gcc/c-family/c.opt:943:
+fpph-dump-tree
+C++ Var(flag_pph_dump_tree)
+-fpph-dump-treeDump global namespace tree around PPH reads/writes.
+

We should just add to the existing -fdump-translation-unit.  Maybe not
so important now.

http://codereview.appspot.com/4431071/diff/1/gcc/cp/name-lookup.c
File gcc/cp/name-lookup.c (right):

http://codereview.appspot.com/4431071/diff/1/gcc/cp/name-lookup.c#newcode1186
gcc/cp/name-lookup.c:1186: {
+tree
+pushdecl_into_namespace (tree dcl, tree nsp)
+{

Needs comment.

http://codereview.appspot.com/4431071/

Re: [pph] Save keyed_classes, unemitted_tinfo_decls, TYPE_BINFO (issue4486042)

[dnovillo]

http://codereview.appspot.com/4486042/diff/1/gcc/cp/pph-streamer.h
File gcc/cp/pph-streamer.h (right):

http://codereview.appspot.com/4486042/diff/1/gcc/cp/pph-streamer.h#newcode152
gcc/cp/pph-streamer.h:152: for (i = 0; i < c; ++i)
+#if 0
+static inline void
+pph_output_tree_array (pph_stream *stream, tree *a, size_t c, bool
ref_p)
+{
+  size_t i;

Why are you adding this #if0 code?  No apparent reason given that we
have pph_stream_write_tree_vec.

http://codereview.appspot.com/4486042/diff/1/gcc/cp/pph-streamer.h#newcode245
gcc/cp/pph-streamer.h:245: tree t;
#if 0
+static inline void
+pph_output_tree_VEC (pph_stream *stream, VEC(tree,gc) *v, bool ref_p)
+{
+  tree t;

Another one of the same?

http://codereview.appspot.com/4486042/diff/1/gcc/cp/pph-streamer.h#newcode338
gcc/cp/pph-streamer.h:338: size_t i;
+static inline void
+pph_input_tree_array (pph_stream *stream, tree *a, size_t c)
+{
+  size_t i;

Likewise.

http://codereview.appspot.com/4486042/diff/1/gcc/cp/pph-streamer.h#newcode355
gcc/cp/pph-streamer.h:355: size_t i;
+#if 0
+static inline void
+pph_input_tree_VEC (pph_stream *stream, VEC(tree,gc) *v)
+{
+  size_t i;

Likewise.

http://codereview.appspot.com/4486042/

Re: [pph] Various Tree Fields (issue4550064)

[dnovillo]

http://codereview.appspot.com/4550064/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/4550064/diff/1/gcc/cp/pph-streamer-in.c#newcode251
gcc/cp/pph-streamer-in.c:251: {
+static VEC(qualified_typedef_usage_t,gc) *
+pph_stream_read_qual_use_vec (pph_stream *stream)
+{

This breaks bootstrap.  This function is never used.  I think you meant
to call it from the TEMPLATE_INFO handler, right?

Does this look like the right fix?


diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index d40fd17..c426466 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -904,7 +904,7 @@ pph_stream_read_tree (struct lto_input_block *ib
ATTRIBUTE_UNUSED,
   else if (TREE_CODE (expr) == TEMPLATE_INFO)
 {
   TI_TYPEDEFS_NEEDING_ACCESS_CHECKING (expr)
-  = pph_stream_read_tree_vec (stream);
+  = pph_stream_read_qual_use_vec (stream);
 }
   else if (TREE_CODE (expr) == TREE_LIST)
 ; /* FIXME pph: already handled?  */

http://codereview.appspot.com/4550064/

Re: [pph] More C++ Tree Nodes (issue4526083)

[dnovillo]

http://codereview.appspot.com/4526083/diff/1/gcc/cp/cp-objcp-common.c
File gcc/cp/cp-objcp-common.c (right):

http://codereview.appspot.com/4526083/diff/1/gcc/cp/cp-objcp-common.c#newcode103
gcc/cp/cp-objcp-common.c:103:
 case TEMPLATE_INFO: return sizeof (struct
tree_template_info);

+case TREE_BINFO:return sizeof (struct tree_binfo);
+


TREE_BINFO is not a C++ code and it's variably-sized.  It is already
handled by tree.c:tree_size()

Not sure why you needed to add this here?

http://codereview.appspot.com/4526083/

Re: [google] Allow relative profile paths. (issue4280074)

[dnovillo]

OK.  A nit and a question below.


Diego.


http://codereview.appspot.com/4280074/diff/1/gcc/doc/invoke.texi
File gcc/doc/invoke.texi (right):

http://codereview.appspot.com/4280074/diff/1/gcc/doc/invoke.texi#newcode
gcc/doc/invoke.texi:: and its related options. Both absolute and
relative can be used.
s/relative/relative paths/

http://codereview.appspot.com/4280074/diff/1/gcc/libgcov.c
File gcc/libgcov.c (right):

http://codereview.appspot.com/4280074/diff/1/gcc/libgcov.c#newcode286
gcc/libgcov.c:286: if (prefix_length != 0 && !IS_DIR_SEPARATOR (*fname))
I'm confused.  why did you take !HAS_DRIVE_SPEC() out?

http://codereview.appspot.com/4280074/

Re: [pph] Test PPH include at global scope. (issue4399041)

[dnovillo]

On 2011/04/12 21:40:10, Lawrence Crowl wrote:

Add a test to ensure that PPH files are #included at global scope.
Initially, this test is XFAIL, as it's a low priority error.



Index: gcc/testsuite/ChangeLog.pph



2011-04-12  Lawrence Crowl  



* g++.dg/pph/y2smother.cc: New.


OK.


Diego.

http://codereview.appspot.com/4399041/

Re: [pph] Renaming output/write and input/read to out/in + standardizing pph_stream_* to pph_* (issue4532102)

[dnovillo]

Looks OK.  One minor formatting comment that I will fix myself when I
commit the patch.


Diego.


http://codereview.appspot.com/4532102/diff/1/pph-streamer-in.c
File pph-streamer-in.c (right):

http://codereview.appspot.com/4532102/diff/1/pph-streamer-in.c#newcode42
pph-streamer-in.c:42: pph_register_shared_data (STREAM, DATA, IX);  \
   (DATA) = (ALLOC_EXPR);   \
-  pph_stream_register_shared_data (STREAM, DATA, IX);  \
+  pph_register_shared_data (STREAM, DATA, IX); \

Align the '\' with the ones above.

http://codereview.appspot.com/4532102/

Re: [pph] Renaming output/write and input/read to out/in + standardizing pph_stream_* to pph_* (issue4532102)

[dnovillo]

On 2011/06/01 13:06:15, Diego Novillo wrote:

Looks OK.  One minor formatting comment that I will fix myself when I

commit the

patch.


Committed as rev 174530.


Diego.

http://codereview.appspot.com/4532102/

Re: [google] Enhance Annotalysis to support cloned functions/methods (issue4591066)

[dnovillo]

OK with some minor nits.


Diego.


http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c
File gcc/tree-threadsafe-analyze.c (right):

http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c#newcode1159
gcc/tree-threadsafe-analyze.c:1159: gcc_assert (false);

+ else
+  gcc_assert (false);
+


Change to gcc_unreachable ();

http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c#newcode2151
gcc/tree-threadsafe-analyze.c:2151: && gimple_call_num_args(call) > 0)
2147  starting from GCC-4.5.). The clones could be created as early
as when
 2148  constructing SSA. Also note that the parameters of a cloned
method could
 2149  be optimized away.  */
 2150   if (TREE_CODE (TREE_TYPE (DECL_ORIGIN (fdecl))) == METHOD_TYPE
 2151   && gimple_call_num_args(call) > 0)

Wouldn't it be easier to make fdecl == DECL_ORIGIN (fdecl) earlier in
the function?

It's OK either way, though.

http://codereview.appspot.com/4591066/

Re: [pph] Rename two pph_start_record functions adding in/out. (issue4629049)

[dnovillo]

On 2011/06/17 23:18:21, Gabriel Charette wrote:


2011-06-17  Gabriel Charette  



* gcc/cp/pph-streamer-in.c (pph_in_start_record):
Rename from pph_start_record. Update all users.
* gcc/cp/pph-streamer-out.c (pph_out_start_record):
Rename from pph_start_record. Update all users.


OK.  Applied to branch.


Diego.

http://codereview.appspot.com/4629049/

Re: [pph] Reorganize pph read/write file into their respective streamers (issue4657042)

[dnovillo]

On 2011/06/21 18:56:24, Gabriel Charette wrote:


2011-06-21  Gabriel Charette  



* gcc/cp/pph-streamer-in.c (pph_in_tree_vec): Make static.
(pph_add_names_to_namespace): Moved from pph.c.
(wrap_macro_def): Moved from pph.c.
(report_validation_error): Moved from pph.c.
(pth_load_identifiers): Moved from pph.c.
(pph_read_file_contents): Moved from pph.c.
(pph_read_file): Moved from pph.c
* gcc/cp/pph-streamer-out.c (pph_out_tree_vec): Make static.
(pph_out_chain_filtered): Make static.
(pth_save_identifiers): Moved from pph.c.
(pph_write_file_contents): Moved from pph.c.
(pph_write_file): Moved from pph.c.
* gcc/cp/pph-streamer.h (pph_out_tree_vec): Now static, removed.
(pph_out_chain_filtered): Now static, removed.
(pph_write_file): Now public, added.
(pph_in_tree_vec): Now static, removed.
(pph_read_file): Now public, added.
* gcc/cp/pph.h (pph_dump_namespace): Exposed.


OK.  Applied to branch.


Diego.

http://codereview.appspot.com/4657042/

Re: [pph] Initialize cache_ix in all paths in pph_start_record (issue4642045)

[dnovillo]

On 2011/06/18 01:29:31, Gabriel Charette wrote:


2011-06-17  Gabriel Charette  



* gcc/cp/pph-streamer-in.c (pph_start_record):
Initialize cache_ix in all paths.


OK.  Applied to branch.


Diego.

http://codereview.appspot.com/4642045/

Re: [pph] Stream scope_chain->bindings instead of global namespace (issue4661045)

[dnovillo]

http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-in.c#newcode1003
gcc/cp/pph-streamer-in.c:1003: namespace.
 1001   /* FIXME pph: this carried over from
pph_add_names_to_namespace,
 1002 »it only makes sense to use this when merging names in an
existing
 1003 »namespace.

pph_add_names_to_namespace does not exist anymore.  I don't understand
this comment.

http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-out.c
File gcc/cp/pph-streamer-out.c (right):

http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-out.c#newcode947
gcc/cp/pph-streamer-out.c:947: gcc_assert ( ss->old_namespace ==
global_namespace
  945   /* old_namespace should be global_namespace and all entries
listed below
  946  should be NULL or 0; otherwise the header parsed was
incomplete.  */
  947   gcc_assert ( ss->old_namespace == global_namespace

No space after '('.

http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-out.c#newcode949
gcc/cp/pph-streamer-out.c:949: || ss->function_decl ||
ss->template_parms || ss->x_saved_tree
  948   && !(ss->class_name || ss->class_type ||
ss->access_specifier
  949|| ss->function_decl || ss->template_parms ||
ss->x_saved_tree

Align '&&' vertically with the open brace.

http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer.c
File gcc/cp/pph-streamer.c (right):

http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer.c#newcode34
gcc/cp/pph-streamer.c:34: #include "name-lookup.h"
  33 #include "cppbuiltin.h"
+ 34 #include "name-lookup.h"

You also need to add cp/name-lookup.h to the list of dependencies for
cp/pph.o in cp/Make-lang.in.

http://codereview.appspot.com/4661045/

Re: [pph] Fix binding_level's names_size streaming (issue4634071)

[dnovillo]

On 2011/06/24 17:35:51, Gabriel Charette wrote:

This was commited to trunk. Diego can you commit this patch to pph as

well?

Done.  r175387.


Diego.

http://codereview.appspot.com/4634071/

Re: [pph] Moved token cache streaming to in/out respectively (issue4635073)

[dnovillo]

On 2011/06/27 18:51:22, Gabriel Charette wrote:


2011-06-27  Gabriel Charette  



* pph-streamer-in.c (pth_get_type_from_index): Moved from pph.c.
(pth_load_number): Moved from pph.c.
(pth_load_token_value): Moved from pph.c.
(pth_load_token): Moved from pph.c.
(pth_load_token_cache): Moved from pph.c. Made static.
* pph-streamer-out.c (pth_get_index_from_type): Moved from pph.c.
(pth_write_number): Moved from pph.c.
(pth_save_token_value): Moved from pph.c.
(pth_save_token): Moved from pph.c.
(pth_save_token_cache): Moved from pph.c. Made static.
* pph-streamer.h (struct cp_token_cache):
Not declared in pph.c, but in parser.h, removed.
(pth_save_token_cache): Now static, removed.
(pth_load_token_cache): Now static, removed.


OK.  Will apply shortly.


Diego.

http://codereview.appspot.com/4635073/

Re: [pph] Rename token cache and identifiers streaming functions to reflect pph naming scheme (issue4636065)

[dnovillo]

On 2011/06/27 20:33:26, Gabriel Charette wrote:

2011-06-27  Gabriel Charette  


Remove the 'mailto:' prefix.



* pph-streamer-in.c (pph_get_type_from_index):
Rename from pth_get_type_from_index. Update all users.
(pph_in_number): Rename from pth_load_number.
Update all users.
(pph_in_token_value): Rename from pth_load_token_value. Change
signature to 'pph_stream*, cp_token*' from 'cp_token*, pph_stream*'.
Update all users.
(pph_in_token): Rename from pth_load_token.
Update all users.
(pph_in_token_cache): Rename from pth_load_token_cache.
Update all users.
(pph_in_identifiers): Rename from pth_load_identifiers. Change
signature to 'pph_stream*, cpp_idents_used*' from
'cpp_idents_used*, pph_stream*'.
from (cpp_idents_used*, pph_stream*).
Update all users.
* pph-streamer-out.c (pph_get_index_from_type):
Rename from pth_get_index_from_type.
Update all users.
(pph_out_number): Rename from pth_write_number.
Update all users.
(pph_out_token_value): Rename from pth_save_token_value.
Update all users.
(pph_out_token): Rename from pth_save_token. Change signature to
'pph_stream*, cp_token*' from 'cp_token*, pph_stream*'.
Update all users.
(pph_out_token_cache): Rename from pth_save_token_cache. Change
signature to 'pph_stream*, cp_token_cache*'
from 'cp_token_cache*, pph_stream*'.
Update all users.
(pph_out_identifiers): Rename from pth_save_identifiers. Change
signature to 'pph_stream*, cpp_idents_used*' from
'cpp_idents_used*, pph_stream*'.
Update all users.


OK with that change.


Diego.

http://codereview.appspot.com/4636065/

Re: [pph] Fix var order when streaming in. (issue4635074)

[dnovillo]

On 2011/06/28 00:27:04, Gabriel Charette wrote:

The names and namespaces chains are built by adding each new element

to the

front of the list. When streaming it in we traverse the list of names

and re-add

them to the current chains; thus reversing the order in which they

were defined

in the header file.



Since this is a singly linked-list we cannot start from the tail; thus

we

reverse the chain in place and then traverse it, now adding the

bindings in the

same order they were found in the header file.



I introduced a new failing test to test this. The test showed the

reverse

behaviour prior to the patch.
The test still fails however, there is another inversion problem

between the

global variables and the .LFBO, .LCFI0, ...
This patch only fixes the inversion of the global variables

declarations in the

assembly, not the second issue this is exposing.
This second issue is potentially already exposed by another test?? Do

we need

this new test?


It can't hurt.


This fixes all of the assembly mismatches in c1limits-externalid.cc

however!

Nice!


2011-06-27  Gabriel Charette  



* pph-streamer-in.c (pph_add_bindings_to_namespace): Reverse names

and

namespaces chains.



* g++.dg/pph/c1limits-externalid.cc: Remove pph asm xdiff.
* g++.dg/pph/c1varorder.cc: New.
* g++.dg/pph/c1varorder.h: New.
* g++.dg/pph/pph.map: Add c1varorder.h


OK with a minor comment nit.

http://codereview.appspot.com/4635074/

Re: [pph] Fix var order when streaming in. (issue4635074)

[dnovillo]

http://codereview.appspot.com/4635074/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/4635074/diff/1/gcc/cp/pph-streamer-in.c#newcode1144
gcc/cp/pph-streamer-in.c:1144: /* The chains are built backwards (ref:
add_decl_to_level@name-lookup.c),

1143
1144   /* The chains are built backwards (ref:

add_decl_to_level@name-lookup.c),

s/add_decl_to_level@name-lookup.c/add_decl_to_level/

http://codereview.appspot.com/4635074/

Re: [pph] Fixing string streaming functions and their comments (issue4654076)

[dnovillo]

On 2011/06/30 01:37:59, Gabriel Charette wrote:


2011-06-29  Gabriel Charette  



* pph-streamer.h (struct pph_stream): Fix comment of data_in field.
(pph_out_string_with_length): lto_output_string_with_length now

handles

NULL strings, call it directly.
(pph_in_string): Fix comment.


OK.


Diego.

http://codereview.appspot.com/4654076/

Re: [pph] Make pph.h _the_ interface header. (issue 5247044)

[dnovillo]

http://codereview.appspot.com/5247044/diff/1/gcc/cp/pph-streamer.h
File gcc/cp/pph-streamer.h (right):

http://codereview.appspot.com/5247044/diff/1/gcc/cp/pph-streamer.h#newcode165
gcc/cp/pph-streamer.h:165: struct pph_stream {
165 struct pph_stream {

You need the typedef.  Stage 1 is still built with C.

http://codereview.appspot.com/5247044/

Re: [google] AddressSanitizer for gcc, first attempt. (issue 5272048)

[dnovillo]

First round of comments.

I think we should add this to google/main.  It's in sufficiently good
shape for it.  You can keep improving it in the branch.

It is now too late for 4.7's stage 1, so I think a reasonable way to
proceed is to keep it in google/main and then present it for trunk
inclusion when stage 1 opens for 4.8.


http://codereview.appspot.com/5272048/diff/30001/toplev.c
File toplev.c (right):

http://codereview.appspot.com/5272048/diff/30001/toplev.c#newcode621
toplev.c:621: asan_finish_file();
+  /* File-scope initialization for AddressSanitizer. */

Two spaces before '*/'

+  if (flag_asan)
+asan_finish_file();

Space before '()'

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c
File tree-asan.c (right):

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode80
tree-asan.c:80: (All I need is to traverse *all* memory accesses and
instrument them).
+ Implementation details:
+ This is my first code in gcc. I wrote it by copying tree-mudflap.c,
+ stripping 70% of irrelevant code and modifying the instrumentation
routine
+ build_check_stmt. The code seems to work, but I don't feel I
understand it.
+ In particular, transform_derefs() and transform_statements() seem too
complex.
+ Suggestions are welcome on how to simplify them.
+ (All I need is to traverse *all* memory accesses and instrument them).

No need to have this in the code.  These details usually go in the mail
message you submit your patch with.

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode140
tree-asan.c:140: 'size' is one of 1, 2, 4, 8, 16.  */
+/* Instrument the memory access instruction 'base'.
+ Insert new statements before 'instr_gsi'.
+ 'location' is source code location.
+ 'is_store' is either 1 (for a store) or 0 (for a load).
+ 'size' is one of 1, 2, 4, 8, 16.  */

When documenting arguments, you should refer to the arguments in
CAPITALS instead of 'quoted'.  The comment needs to be formatted so the
lines below the first line are indented 3 spaces.

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode155
tree-asan.c:155: tree shadow_type = size > 8 ? short_integer_type_node :
char_type_node;
+  tree shadow_type = size > 8 ? short_integer_type_node :
char_type_node;

s/8/BITS_PER_UNIT/

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode219
tree-asan.c:219: build1 (VIEW_CONVERT_EXPR, shadow_ptr_type, t));
+  t = build2 (RSHIFT_EXPR, uintptr_type, base_addr,
+  build_int_cst (uintptr_type, asan_scale));
+  t = build2 (PLUS_EXPR, uintptr_type, t,
+  build2 (LSHIFT_EXPR, uintptr_type,
+  build_int_cst (uintptr_type, 1),
+  build_int_cst (uintptr_type, asan_offset_log)
+ ));
+  t = build1 (INDIRECT_REF, shadow_type,
+  build1 (VIEW_CONVERT_EXPR, shadow_ptr_type, t));

It helps if this adds documentation on what expressions it's building.

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode250
tree-asan.c:250: gimple_seq_add_stmt (&seq, g);
[ ... ]
+  g = gimple_build_assign  (cond, t);
+  gimple_set_location (g, location);
+  gimple_seq_add_stmt (&seq, g);
+  g = gimple_build_cond (NE_EXPR, cond, boolean_false_node, NULL_TREE,
+ NULL_TREE);
+  gimple_set_location (g, location);
+  gimple_seq_add_stmt (&seq, g);

So, instead of open coding all the access checks, how about we created a
new GIMPLE instruction?  This instruction would take the same
parameters, and have the semantics of the check but it would be
optimizable.  You could for instance make the instruction produce a
pointer SSA name that is then used by the memory reference.

With this, you can then allow the optimizers do their thing.  These
instructions could be moved around, eliminated, coalesced.  Resulting in
a reduced number of checks.

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode251
tree-asan.c:251: /* debug_gimple_seq (seq);  */
+  /* debug_gimple_seq (seq);  */

If you want the pass to dump debugging data, use the dump support.  See
other passes for examples (look for 'if (dump_file)' snippets)'.  When
-fdump-tree-asan is passed to the compiler, the pass manager will
instantiate a 'dump_file' that you can use to emit debug info.

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode253
tree-asan.c:253: /* crash  */
+  /* crash  */

???

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode272
tree-asan.c:272: location_t location, int is_store)
+static void
+transform_derefs (gimple_stmt_iterator *iter, tree *tp,
+   location_t location, int is_store)

Needs documentation.

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode326
tree-asan.c:326: bb = ENTRY_BLOCK_PTR ->next_bb;
+  bb = ENTRY_BLOCK_PTR ->next_bb;

No space before '->'.

http://codereview.appspot.com/5272048/diff/30001/tree-asan.c#newcode327
tree-asan.c:327: do
FOR_EACH_BB (bb)

http:

Re: [google] AddressSanitizer for gcc, first attempt. (issue 5272048)

[dnovillo]

The invoke.texi change looks fine.  The ChangeLog entry needs some work.


http://codereview.appspot.com/5272048/diff/41001/ChangeLog.google-main
File ChangeLog.google-main (right):

http://codereview.appspot.com/5272048/diff/41001/ChangeLog.google-main#newcode6
ChangeLog.google-main:6:
 1 2011-11-02   Kostya Serebryany  
2 Introduce a new option -fasan which enables
3 AddressSanitizer, a fast memory error detector:
4 http://code.google.com/p/address-sanitizer.
5 The current implementation handles only heap accesses.
6

Not quite.  A ChangeLog entry needs to have a rigid structure.  For
every file and function changed, you need to state what changed.  The
entry below this one is not really a good example.   ChangeLog entries
never describe how the patch works or what it provides.  See examples in
gcc/ChangeLog.

See
http://www.gnu.org/prep/standards/standards.html#Style-of-Change-Logs
for details.

http://codereview.appspot.com/5272048/

Re: [google] AddressSanitizer for gcc, first attempt. (issue 5272048)

[dnovillo]

OK for google/main with the nits below.



http://codereview.appspot.com/5272048/diff/42003/ChangeLog.google-main
File ChangeLog.google-main (right):

http://codereview.appspot.com/5272048/diff/42003/ChangeLog.google-main#newcode1
ChangeLog.google-main:1: 2011-11-02   Kostya Serebryany

   1 2011-11-02   Kostya Serebryany  

Need blank line below the datestamp.

http://codereview.appspot.com/5272048/diff/42003/ChangeLog.google-main#newcode3
ChangeLog.google-main:3: AddressSanitizer, a fast memory error detector:
2 Introduce a new option -fasan which enables
3 AddressSanitizer, a fast memory error detector:

Not strictly accepted, but it is common to add some verbiage of this
nature when adding major features.  OK.

http://codereview.appspot.com/5272048/

Re: [pph] Expect checksums for tests marked with pph asm xdiff (issue4744043)

[dnovillo]

OK with the change below.


Diego.


http://codereview.appspot.com/4744043/diff/3001/gcc/testsuite/lib/dg-pph.exp
File gcc/testsuite/lib/dg-pph.exp (right):

http://codereview.appspot.com/4744043/diff/3001/gcc/testsuite/lib/dg-pph.exp#newcode131
gcc/testsuite/lib/dg-pph.exp:131: set adiff [catch {exec diff
"$bname.s-pph" "$bname.s+pph"} diff_result]
131 set adiff [catch {exec diff "$bname.s-pph" "$bname.s+pph"}
diff_result]

You don't need this, if instead of summing the diff you sum
$bname.s+pph.  The logic would then be: if there is a difference between
$bname.s-pph and $bname.s+pph, we checksum $bname.s+pph.  If the new
checksum is different than the stored one, it means that $bname.s+pph is
different from $bname.s-pph in a different way.

This has the benefit of:

- Being slightly faster.
- Simplifies the generation of checksums.  We no longer need to checksum
the difference between the two .s files, we just need to checksum the
.s+pph file

http://codereview.appspot.com/4744043/

Re: [pph] Add filter to prevent streaming out builtin identifiers (issue4802047)

[dnovillo]

On 2011/07/20 20:54:31, Gabriel Charette wrote:


Should I instead add a new macro IS_BUILTIN or something like that to

encompass

that logic?


Sure.  But make it a static inline function, please.

OK with that change.


Diego.

http://codereview.appspot.com/4802047/

Re: [pph] Stream and merge line table. (issue4836050)

[dnovillo]

OK with these changes.

As far as the trunk changes go.  Just commit your changes to the branch.
 I will get the trunk changes whenever they get approved in some future
merge.


Diego.


http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer-in.c#newcode73
gcc/cp/pph-streamer-in.c:73: int pph_loc_offset;
   71 /* Set in pph_in_and_merge_line_table. Represents the
source_location offset
   72which every streamed in token must add to it's serialized
source_location. */
   73 int pph_loc_offset;

Make it static.

http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer-out.c
File gcc/cp/pph-streamer-out.c (right):

http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer-out.c#newcode134
gcc/cp/pph-streamer-out.c:134: simply add them to the cache in the
preload.  */
  132   /* FIXME pph: we are streaming builtin locations, which implies
that we are
  133  streaming some builtins, we probably want to figure out what
those are and
  134  simply add them to the cache in the preload.  */

Hmm, we are streaming builtins?  The low-level streamer detects builtins
and only emits the builtin code, not the whole tree.  What builtins are
getting through?

This can be addressed on a follow-up patch.  It just sounds strange to
me that we are streaming builtins and locations.

http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer.h
File gcc/cp/pph-streamer.h (right):

http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer.h#newcode344
gcc/cp/pph-streamer.h:344: streamer hook back to pph_write_location.  */
 342FIXME pph: If pph_trace didn't depend on STREAM, we could avoid
having to
 343call this function, only for it to call lto_output_location,
which calls the
 344streamer hook back to pph_write_location.  */

Just call pph_write_location here.  No need to call lto_output_location
anymore.   We only rely on lto_output_location calling us when it's
called from tree leaves inside the tree streamer routines.

http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer.h#newcode419
gcc/cp/pph-streamer.h:419:
415 /* Read and return a location_t from STREAM.
 416FIXME pph: If pph_trace didn't depend on STREAM, we could avoid
having to
 417call this function, only for it to call lto_input_location,
which calls the
 418streamer hook back to pph_read_location.  */
 419

Likewise here.

http://codereview.appspot.com/4836050/diff/1/gcc/lto-streamer-in.c
File gcc/lto-streamer-in.c (right):

http://codereview.appspot.com/4836050/diff/1/gcc/lto-streamer-in.c#newcode342
gcc/lto-streamer-in.c:342: if(streamer_hooks.input_location)
  342   if(streamer_hooks.input_location)

Space before '('

http://codereview.appspot.com/4836050/

Re: Run ThreadSanitizer test only on x86/linux (issue 5437087)

[dnovillo]

On 2011/11/30 14:08:34, dvyukov wrote:

The patch is for google-main branch.
Add directives to run ThreadSanitizer tests
only on i386/x86_64-*-linux targets.



Index: gcc/ChangeLog.google-main
===
--- gcc/ChangeLog.google-main   (revision 181841)
+++ gcc/ChangeLog.google-main   (working copy)
@@ -1,3 +1,11 @@
+2011-11-30   Dmitriy Vyukov  
+
+   Add directives to run ThreadSanitizer tests
+   only on i386/x86_64-*-linux targets.
+   * gcc/testsuite/gcc.dg/tsan-ignore.c: Add the directives.
+   * gcc/testsuite/gcc.dg/tsan-stack.c: Add the directives.
+   * gcc/testsuite/gcc.dg/tsan-mop.c: Add the directives.
+
  2011-11-23   Teresa Johnson  



* loop-unroll.c (loop_has_FP_comp): New function.
Index: gcc/testsuite/gcc.dg/tsan-ignore.c
===
--- gcc/testsuite/gcc.dg/tsan-ignore.c  (revision 181841)
+++ gcc/testsuite/gcc.dg/tsan-ignore.c  (working copy)
@@ -1,4 +1,4 @@
-/* { dg-do run } */
+/* { dg-do run { target i?86-*-linux* x86_64-*-linux* } } */
  /* { dg-options "-ftsan -O1 -ftsan-ignore=tsan-ignore.ignore" } */
  #include "tsan.h"
  #include "tsan-ignore.h"
Index: gcc/testsuite/gcc.dg/tsan-stack.c
===
--- gcc/testsuite/gcc.dg/tsan-stack.c   (revision 181841)
+++ gcc/testsuite/gcc.dg/tsan-stack.c   (working copy)
@@ -1,4 +1,4 @@
-/* { dg-do run } */
+/* { dg-do run { target i?86-*-linux* x86_64-*-linux* } } */
  /* { dg-options "-ftsan -O1" } */
  #include "tsan.h"



Index: gcc/testsuite/gcc.dg/tsan-mop.c
===
--- gcc/testsuite/gcc.dg/tsan-mop.c (revision 181841)
+++ gcc/testsuite/gcc.dg/tsan-mop.c (working copy)
@@ -1,4 +1,4 @@
-/* { dg-do run } */
+/* { dg-do run { target i?86-*-linux* x86_64-*-linux* } } */
  /* { dg-options "-ftsan -O1" } */
  #include "tsan.h"




--
This patch is available for review at

http://codereview.appspot.com/5437087

This is OK.


Diego.

http://codereview.appspot.com/5437087/

Re: Remove LINEMAP_POSITION_FOR_COLUMN macro (issue 4874043)

[dnovillo]

On 2011/08/15 18:05:51, Gabriel Charette wrote:


2011-08-11  Gabriel Charette  



 libcpp/ChangeLog
* include/line-map.h (LINEMAP_POSITION_FOR_COLUMN): Remove.
Update all users to use linemap_position_for_column instead.



 gcc/go/ChangeLog
* gofrontend/lex.cc (Lex::location): Update to use
linemap_position_for_column instead.
 (Lex::earlier_location): Likewise.


This is OK.  The changes to Go are just a side-effect of the other
changes and they are small enough that they should be easy to rollback.


Diego.

http://codereview.appspot.com/4874043/

Re: [pph] Fix child references being included multiple times (issue 4904050)

[dnovillo]

OK with a minor nit.


http://codereview.appspot.com/4904050/diff/3001/gcc/cp/pph-streamer.h
File gcc/cp/pph-streamer.h (right):

http://codereview.appspot.com/4904050/diff/3001/gcc/cp/pph-streamer.h#newcode210
gcc/cp/pph-streamer.h:210: extern VEC(pph_stream_ptr, heap)
*pph_read_images;

209
210 extern VEC(pph_stream_ptr, heap) *pph_read_images;


Move earlier in the file and add documentation.

http://codereview.appspot.com/4904050/

Re: [pph] Add support for line table streaming with includes (issue 4908051)

[dnovillo]

Very nice.  One potential application of this in the future would be to
not only sequence the included files, but also the symbols and types.
To support the cases where a child include depends on symbols exported
by the parent before its inclusion (though I'm not sure we want to
really support that).

OK with the changes below.


http://codereview.appspot.com/4908051/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/4908051/diff/1/gcc/cp/pph-streamer-in.c#newcode1402
gcc/cp/pph-streamer-in.c:1402: gcc_assert(lm->included_from == -1);
Space before '('

http://codereview.appspot.com/4908051/diff/1/gcc/cp/pph-streamer-out.c
File gcc/cp/pph-streamer-out.c (right):

http://codereview.appspot.com/4908051/diff/1/gcc/cp/pph-streamer-out.c#newcode1211
gcc/cp/pph-streamer-out.c:1211: header_name = header_path + 1;
Actually, you can use lbasename from libiberty.  You can then figure out
the extension with strrchr.  I forgot to tell you about it, sorry.

http://codereview.appspot.com/4908051/diff/1/gcc/cp/pph-streamer-out.c#newcode1328
gcc/cp/pph-streamer-out.c:1328: #ifdef and only enabled if asserts are
on.  */
Actually, leave it in permanently.  Changing the output of the compiler
depending on whether checking is enabled is bad ju-ju.  An extra int at
the end of the line table will not hurt.

http://codereview.appspot.com/4908051/

Re: [pph] Independent pre-loaded cache for common nodes (issue 4956041)

[dnovillo]

OK with a couple of nits below.


Diego.


http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer-in.c#newcode155
gcc/cp/pph-streamer-in.c:155: || marker == PPH_RECORD_PREF;
154   return marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF
155  || marker == PPH_RECORD_PREF;

Align these vertically.

http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer-in.c#newcode197
gcc/cp/pph-streamer-in.c:197: || marker == PPH_RECORD_PREF)
154   return marker == PPH_RECORD_IREF || marker == PPH_RECORD_XREF
155  || marker == PPH_RECORD_PREF;

Likewise.

http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer.c
File gcc/cp/pph-streamer.c (right):

http://codereview.appspot.com/4956041/diff/1/gcc/cp/pph-streamer.c#newcode530
gcc/cp/pph-streamer.c:530: enum pph_record_marker marker)
529 pph_cache_get (pph_pickle_cache *stream_cache, unsigned include_ix,
unsigned ix,
 530enum pph_record_marker marker)

Do we really need the MARKER argument?  If STREAM_CACHE is NULL, we use
INCLUDE_IX to determine if they want an external reference or the
preloaded cache.

Though, I guess adding MARKER does not hurt and makes the code easier to
read.  Never mind.

http://codereview.appspot.com/4956041/

Re: [pph] Fix pph_read_tree_header. (issue 5050045)

[dnovillo]

http://codereview.appspot.com/5050045/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/5050045/diff/1/gcc/cp/pph-streamer-in.c#newcode2024
gcc/cp/pph-streamer-in.c:2024: /* Read the language-independent
bitfields for expr.  */
s/expr/EXPR/

http://codereview.appspot.com/5050045/diff/1/gcc/cp/pph.c
File gcc/cp/pph.c (right):

http://codereview.appspot.com/5050045/diff/1/gcc/cp/pph.c#newcode50
gcc/cp/pph.c:50: gcc_assert (code <= TEMPLATE_INFO);
  48 pph_tree_code_text (enum tree_code code)
  49 {
  50   gcc_assert (code <= TEMPLATE_INFO);

gcc_assert (code < MAX_TREE_CODES);

http://codereview.appspot.com/5050045/

Re: [pph] Stream merging information (issue 5090041)

[dnovillo]

http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-in.c#newcode2146
gcc/cp/pph-streamer-in.c:2146: pph_read_namespace_chain (pph_stream
*stream, tree enclosing_namespace)
 2142 /* Read a chain of tree nodes from input block IB. DATA_IN
contains
 2143tables and descriptors for the file being read.  */
 2144
 2145 tree
 2146 pph_read_namespace_chain (pph_stream *stream, tree
enclosing_namespace)

ENCLOSING_NAMESPACE needs documenting.  Would it be better to have the
original pph_read_chain() get this argument?  Not crazy about this
duplication of code.

Same comment applies to the other two routines that the patch
duplicates.

http://codereview.appspot.com/5090041/

Re: [pph] Stream merging information (issue 5090041)

[dnovillo]

http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-out.c
File gcc/cp/pph-streamer-out.c (right):

http://codereview.appspot.com/5090041/diff/1/gcc/cp/pph-streamer-out.c#newcode1924
gcc/cp/pph-streamer-out.c:1924: tree enclosing_namespace )
 1922 void
 1923 pph_write_namespace_tree (pph_stream *stream, tree expr,
 1924   tree enclosing_namespace )

Just found this while changing other code.  This needs a comment.  no
space before ')'.

http://codereview.appspot.com/5090041/