On 2012/02/01 18:05:15, Diego Novillo wrote:
On 2/1/12 3:16 AM, Dmitriy Vyukov wrote:
> This is for google/gcc-4_6 branch.
> Backport ThreadSanitizer (tsan) instrumentation pass from
google/main.
Have you used the validator script to test this patch? Your patch
should not affect regular builds, but you want to make sure that the
tsan tests don't produce failures in any configuration.
Hi Diego,
Yes, I've tested it with the script with crosstool tester.
PTAL, I've addressed all comments except 2:
> +
> +#include<stdlib.h>
> +#include<stdio.h>
These includes are not needed. They are handled by system.h (GCC
defers
all/most system includes to these files).
> + /* Check if a user has defined it for testing. */
> + id = get_identifier (name);
> + var = varpool_node_for_asm (id);
> + if (var != NULL)
> + {
> + decl = var->decl;
> + gcc_assert (TREE_CODE (decl) == VAR_DECL);
> + return decl;
> + }
> +
> + decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, id, typ);
It would be slightly more useful to give this decl a known location.
How
about passing it in from the caller? DECL_SOURCE_LOCATION
(current_function_decl) may work when you have no other location (like
a
statement or another symbol).
At worst, you pass UNKNOWN_LOCATION when you don't even have that in
the
caller.
Here I create a declaration for a var that is defined in our run-time
library. If I use some real location, then the declaration will have
different irrelevant locations in each TU (irrelevant, because it will
be somewhere near begin of a first instrumented function), + there will
be the definition with a correct location (inside of our run-time
library). Does it make sense? The current situation (a lot of
declarations with unknown location + definition with correct location)
looks OK IMVHO.
> +get_thread_ignore_decl (void)
> +{
> + static tree decl;
> +
> + if (decl == NULL)
> + decl = build_var_decl (integer_type_node, TSAN_IGNORE);
> + return decl;
> +}
> +
> +/* Returns a definition of a runtime functione with type TYP and
name NAME.
*/
s/functione/function/
> + static tree decl;
> +
> + if (decl != NULL)
> + return decl;
> +
> + typ = build_function_type_list (void_type_node, NULL_TREE);
> + decl = build_func_decl (typ, TSAN_INIT);
> + return decl;
> +}
> +
> +/* Adds new ignore definition to the global list.
> + TYPE is the ignore type (see tsan_ignore_type).
The code uses TYPE and TYP. Please change to always use TYPE.
> +}
> +
> +/* Checks as to whether identifier STR matches template TEMPL.
> + Templates can only contain '*', e.g. 'std*string*insert'.
> + Templates implicitly start and end with '*'
> + since they are matched against mangled names.
> + Returns non-zero if STR is matched against TEMPL. */
> +
> +static int
Use 'bool' instead of 'int'.
> + char buf [PATH_MAX];
> +
> + if(getenv("GCCTSAN_PAUSE"))
> + {
> + int res;
> + printf("ATTACH A DEBUGGER AND PRESS ENTER\n");
> + res = scanf("%s", buf);
> + (void)res;
> + }
No debugging getenv(), please. Either use a flag or remove this code.
> + }
> + }
> + if (f == NULL)
> + {
> + error ("failed to open ignore file '%s'\n",
flag_tsan_ignore);
This should be a fatal_error, not error (error is for problems with
the
input code, not the flags).
> + if (do_dec)
> + {
> + size_val = -size_val;
> + size_valhi = -1;
> + }
> + op_size = build_int_cst_wide (sizetype, size_val, size_valhi);
> + sstack_decl = get_shadow_stack_decl ();
> + op_expr = build2 (POINTER_PLUS_EXPR, ptr_type_node, sstack_decl,
op_size);
fold_build2_loc() (likewise in other places). Get the location from
the sequence you are given.
I set location for each inserted gimple separately in:
static void
set_location (gimple_seq seq, location_t loc)
{
gimple_seq_node n;
for (n = gimple_seq_first (seq); n != NULL; n = n->next)
gimple_set_location (n->stmt, loc);
}
It works post factum - I obtain callers pc from the runtime function,
and then obtain debug info the pc, I never saw any bad precedents.
Does it make sense to set location for all tree's? I can't extract
location from the sequence, because it's NULL (however, of course, I
still can pass all the way down).
> + addr_expr = force_gimple_operand (expr_ptr, gseq, true,
NULL_TREE);
> + expr_type = TREE_TYPE (expr);
> + while (TREE_CODE (expr_type) == ARRAY_TYPE)
> + expr_type = TREE_TYPE (expr_type);
> + expr_size = TYPE_SIZE (expr_type);
> + size = tree_to_double_int (expr_size);
> + gcc_assert (size.high == 0&& size.low != 0);
> + if (size.low> 128)
> + size.low = 128;
Could you add a comment here? I'm not sure what this 128 means.
> +/* Checks as to whether EXPR refers to constant var/field/param.
> + Don't bother to instrument them. */
> +
> +static int
> +is_load_of_const (tree expr, int is_store)
> +{
s/int/bool/
> +static void
> +handle_gimple (gimple_stmt_iterator gsi, VEC (mop_desc, heap)
**mop_list)
> +{
> + unsigned i;
> + struct mop_desc mop;
> + gimple stmt;
> + enum gimple_code gcode;
> + tree rhs;
> + tree lhs;
> +
> + stmt = gsi_stmt (gsi);
> + gcode = gimple_code (stmt);
> + if (gcode>= LAST_AND_UNUSED_GIMPLE_CODE)
> + return;
If GCODE is not a valid gimple code, I would assert here. It means
that
something has gone wrong upstream.
> Property changes on: gcc/tree-tsan.c
> ___________________________________________________________________
> Added: svn:eol-style
> + LF
Please remove this attribute. There are other instances of this.
OK with those changes.
Diego.
http://codereview.appspot.com/5610048/
