Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
On 11-11-30 03:53 , dvyu...@google.com wrote: On 2011/11/14 16:48:45, davidxl wrote: Ok for google/main after compiler bootstrap and regression test (without ftsan), and some large tests with tsan turned on (as many as you can but at your discretion). Hi David, Perhaps a bit late question... but better late than never. I've added some unit tests, and I only tested them on Linux/x86 (and it is the only platform that we currently care about). AFAIR I've seen some sort of annotations to restrict tests to particular platforms. Does it make sense to add them to the tests? Yes, please. See http://gcc.gnu.org/onlinedocs/gccint/Directives.html for documentation on the directives you can use. The testsuite/ directory is also full of examples you can cut-n-paste from. Diego.
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
ok for google branches. David On Wed, Nov 30, 2011 at 6:09 AM, dvyu...@google.com wrote: On 2011/11/30 13:17:04, Diego Novillo wrote: On 11-11-30 03:53 , mailto:dvyu...@google.com wrote: On 2011/11/14 16:48:45, davidxl wrote: Ok for google/main after compiler bootstrap and regression test (without ftsan), and some large tests with tsan turned on (as many as you can but at your discretion). Hi David, Perhaps a bit late question... but better late than never. I've added some unit tests, and I only tested them on Linux/x86 (and it is the only platform that we currently care about). AFAIR I've seen some sort of annotations to restrict tests to particular platforms. Does it make sense to add them to the tests? Yes, please. See http://gcc.gnu.org/onlinedocs/gccint/Directives.html for documentation on the directives you can use. The testsuite/ directory is also full of examples you can cut-n-paste from. Here it is http://codereview.appspot.com/5437087/ http://codereview.appspot.com/5303083/
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
Ok for google/main after compiler bootstrap and regression test (without ftsan), and some large tests with tsan turned on (as many as you can but at your discretion). David On Sun, Nov 13, 2011 at 11:59 PM, dvyu...@google.com wrote: On 2011/11/11 00:00:35, davidxl wrote: Have you run through SPEC, and SPEC06 with this change? What is the instrumentation overhead using gcc? David No, I don't use SPEC for testing and benchmarking. For testing I use a pair of large tests in you-know-what codebase. As for benchmarking, overheads are dominated by runtime processing, moderate variations in instrumentation have negligible effect. As of now I would be happy to make it correct and finally commit, probably I spent a way too much time on optimizations already so now it's hard to make it correct and commit. http://codereview.appspot.com/5303083/
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
http://codereview.appspot.com/5303083/diff/28001/gcc/tree-tsan.c File gcc/tree-tsan.c (right): http://codereview.appspot.com/5303083/diff/28001/gcc/tree-tsan.c#newcode227 gcc/tree-tsan.c:227: var = varpool_node_for_asm (id); Use cgraph_node_for_asm instead. http://codereview.appspot.com/5303083/
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
Have you run through SPEC, and SPEC06 with this change? What is the instrumentation overhead using gcc? David http://codereview.appspot.com/5303083/
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
On Thu, Nov 10, 2011 at 4:24 PM, Kostya Serebryany k...@google.com wrote: On Thu, Nov 10, 2011 at 4:00 PM, davi...@google.com wrote: Have you run through SPEC, and SPEC06 with this change? What is the instrumentation overhead using gcc? I don't think anyone of us ever run spec with tsan. Mostly because this will always use the fast path of the tsan analysis (spec is single-threaded). I suggested it because It is good for correctness testing, instrumentation (only) overhead testing. David --kcc David http://codereview.appspot.com/5303083/
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode911 gcc/tree-tsan.c:911: bbd-has_sb = 0; Where is the code that instrument function calls? I do not instrument calls. Instead I instrument func entry/exit. How are calls to locking and synchronization functions tracked? David http://codereview.appspot.com/5303083/
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
On Thu, Nov 3, 2011 at 6:31 AM, dvyu...@google.com wrote:
On 2011/11/01 16:59:04, davidxl wrote:
that means some existing bugs get exposed.
It is quite likely.
Your previous version
simply skipped the target mem refs.
Hummm... how can non-handling of some expressions lead to crashes? I
would expect that it just leads to non 100% memory access coverage...
Likely the way the memop is instrumented is buggy.
You will need to debug the
problem a little more.
What I have is a tree with no type. I have no idea why it happens.
(gdb) print t-common.typed-type
$48 = (tree) 0x0
(gdb) print t-common.typed
$47 = {base = {code = SSA_NAME, side_effects_flag = 0, constant_flag =
0, addressable_flag = 0, volatile_flag = 0, readonly_flag = 0,
unsigned_flag = 0, asm_written_flag = 0, nowarning_flag = 0, used_flag =
0, nothrow_flag = 1,
static_flag = 0, public_flag = 0, private_flag = 0, protected_flag =
0, deprecated_flag = 0, saturating_flag = 0, default_def_flag = 0,
lang_flag_0 = 0, lang_flag_1 = 0, lang_flag_2 = 0, lang_flag_3 = 0,
lang_flag_4 = 0,
lang_flag_5 = 0, lang_flag_6 = 0, visited = 0, packed_flag = 0,
user_align = 0, nameless_flag = 0, expr_folded_flag = 0, spare = 0,
address_space = 0}, type = 0x0}
For debugging purpose, you should use debug print functions in
tree-print.c and tree-pretty-print.c such as debug_tree,
debug_generic_expr, debug_generic_stmt etc to dump trees instead of
using raw gdb print. Here you have a SSA name without type -- it may
be created by the tsan instrumentation code.
David
David
On Tue, Nov 1, 2011 at 5:26 AM, mailto:dvyu...@google.com wrote:
On 2011/10/31 06:08:34, davidxl wrote:
http://codereview.appspot.com/5303083/diff/1/gcc/passes.c#newcode1423
gcc/passes.c:1423: NEXT_PASS (pass_tsan);
Move this to the same place as asan. Otherwise TARGET_MEM_REF won't
be
handled.
After I moved the pass it started crashing:
Program received signal SIGSEGV, Segmentation fault.
0x00718f94 in is_gimple_reg_type (t=0x7771efa0)
nbsp; nbsp;at gimple.c:2960
2960 nbsp; nbsp; nbsp;return !AGGREGATE_TYPE_P (type);
(gdb) bt
#0 nbsp;0x00718f94 in is_gimple_reg_type (t=0x7771efa0)
nbsp; nbsp;at gimple.c:2960
#1 nbsp;is_gimple_val (t=0x7771efa0) at gimple.c:3028
#2 nbsp;0x008a5d20 in verify_types_in_gimple_reference
(expr=0x776b74c0, require_lvalue=false)
nbsp; nbsp;at tree-cfg.c:2934
#3 nbsp;0x008b2d4f in verify_gimple_in_cfg
(fn=0x777c67e0)
nbsp; nbsp;at tree-cfg.c:4382
#4 nbsp;0x00a061d6 in verify_ssa (check_modified_stmt=true)
nbsp; nbsp;at tree-ssa.c:924
#5 nbsp;0x007f755c in execute_function_todo (data=Unhandled
dwarf
expression opcode 0xf3
)
nbsp; nbsp;at passes.c:1727
#6 nbsp;0x007f7e4d in execute_todo (flags=34854) at
passes.c:1758
#7 nbsp;0x007fafda in execute_one_pass (pass=0x122a900)
nbsp; nbsp;at passes.c:2104
The code seems to be (however I am not 100% sure):
nbsp;D.3617_33 = MEM[(const uint64_t
*)ctx_11(D)].nhkey[D.3612_26]{lb:
D.3810_279 sz: 8};
http://codereview.appspot.com/5303083/
http://codereview.appspot.com/5303083/
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
that means some existing bugs get exposed. Your previous version
simply skipped the target mem refs. You will need to debug the
problem a little more.
David
On Tue, Nov 1, 2011 at 5:26 AM, dvyu...@google.com wrote:
On 2011/10/31 06:08:34, davidxl wrote:
http://codereview.appspot.com/5303083/diff/1/gcc/passes.c#newcode1423
gcc/passes.c:1423: NEXT_PASS (pass_tsan);
Move this to the same place as asan. Otherwise TARGET_MEM_REF won't be
handled.
After I moved the pass it started crashing:
Program received signal SIGSEGV, Segmentation fault.
0x00718f94 in is_gimple_reg_type (t=0x7771efa0)
at gimple.c:2960
2960 return !AGGREGATE_TYPE_P (type);
(gdb) bt
#0 0x00718f94 in is_gimple_reg_type (t=0x7771efa0)
at gimple.c:2960
#1 is_gimple_val (t=0x7771efa0) at gimple.c:3028
#2 0x008a5d20 in verify_types_in_gimple_reference
(expr=0x776b74c0, require_lvalue=false)
at tree-cfg.c:2934
#3 0x008b2d4f in verify_gimple_in_cfg (fn=0x777c67e0)
at tree-cfg.c:4382
#4 0x00a061d6 in verify_ssa (check_modified_stmt=true)
at tree-ssa.c:924
#5 0x007f755c in execute_function_todo (data=Unhandled dwarf
expression opcode 0xf3
)
at passes.c:1727
#6 0x007f7e4d in execute_todo (flags=34854) at passes.c:1758
#7 0x007fafda in execute_one_pass (pass=0x122a900)
at passes.c:2104
The code seems to be (however I am not 100% sure):
D.3617_33 = MEM[(const uint64_t *)ctx_11(D)].nhkey[D.3612_26]{lb:
D.3810_279 sz: 8};
http://codereview.appspot.com/5303083/
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
http://codereview.appspot.com/5303083/diff/3002/gcc/tree-tsan.c File gcc/tree-tsan.c (right): http://codereview.appspot.com/5303083/diff/3002/gcc/tree-tsan.c#newcode1075 gcc/tree-tsan.c:1075: for (eidx = 0; VEC_iterate (edge, exit_bb-preds, eidx, e); eidx++) Use FOR_EACH_EDGE macro http://codereview.appspot.com/5303083/diff/3002/gcc/tree-tsan.c#newcode1082 gcc/tree-tsan.c:1082: gsi_insert_seq_before (gsi, post_func_seq, GSI_SAME_STMT); On 2011/11/01 11:39:49, dvyukov wrote: Do I need to make a copy of POST_FUNC_SEQ here? I think that I do not need location info for this code at all, so is it OK to leave the seq w/o location and then insert it into several basic blocks? Yes, do not share gimple stmts. http://codereview.appspot.com/5303083/
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
Hi, sorry that I'm not using the fancy web tool but I do not want to use my google account and gmail address in particular for work-related stuff. On Tue, Nov 01, 2011 at 06:05:46PM +, davi...@google.com wrote: ... http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode638 gcc/tree-tsan.c:638: _vptr., sizeof (_vptr.) - 1) == 0) This is a very hacky way of recognizing vptr field. C++ FE provides TYPE_VFIELD macro to get the vptr field, but you will need to add a new langhook for it -- which is not liked in upstream -- so the hacky way may be ok (as it is for error checking purpose). If you have a FIELD_DECL and want to check whether it is a VPTR, you can simply use DECL_VIRTUAL_P. Martin
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
On 11-11-01 15:26 , Martin Jambor wrote: Hi, sorry that I'm not using the fancy web tool but I do not want to use my google account and gmail address in particular for work-related stuff. No worries. You do not need to use the web tool at all. You can simply reply to these messages. As long as you keep re...@codereview.appspotmail.com in the CC and do not remove the (issue NN) string from the subject, your message will be added to the issue log (similarly to how bugzilla works). Diego.
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
On Tue, Nov 1, 2011 at 12:26 PM, Martin Jambor mjam...@suse.cz wrote: Hi, sorry that I'm not using the fancy web tool but I do not want to use my google account and gmail address in particular for work-related stuff. On Tue, Nov 01, 2011 at 06:05:46PM +, davi...@google.com wrote: ... http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode638 gcc/tree-tsan.c:638: _vptr., sizeof (_vptr.) - 1) == 0) This is a very hacky way of recognizing vptr field. C++ FE provides TYPE_VFIELD macro to get the vptr field, but you will need to add a new langhook for it -- which is not liked in upstream -- so the hacky way may be ok (as it is for error checking purpose). If you have a FIELD_DECL and want to check whether it is a VPTR, you can simply use DECL_VIRTUAL_P. ah yes, that will do. thanks, David Martin
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
Have not done with reviewing. This is the first batch.
David
http://codereview.appspot.com/5303083/diff/1/gcc/passes.c
File gcc/passes.c (right):
http://codereview.appspot.com/5303083/diff/1/gcc/passes.c#newcode1423
gcc/passes.c:1423: NEXT_PASS (pass_tsan);
Move this to the same place as asan. Otherwise TARGET_MEM_REF won't be
handled.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c
File gcc/tree-tsan.c (right):
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode56
gcc/tree-tsan.c:56: The instrumentation module mainintains shadow call
stacks
s/mainitains/maintains/
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode60
gcc/tree-tsan.c:60: Instrumentation for shadow stack maintainance is as
follows:
s/maintainance/maintenance/
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode94
gcc/tree-tsan.c:94: #define RTL_STACK __tsan_shadow_stack
Please change RTL_ prefix to TSAN_. It is confusing to use RTL_
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode100
gcc/tree-tsan.c:100: enum tsan_ignore_e
better to be tsan_ignore_type or tsan_ignore_kind.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode110
gcc/tree-tsan.c:110: enum bb_state_e
A new empty line is needed. Same for other comments leading a decl, or
function.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode110
gcc/tree-tsan.c:110: enum bb_state_e
bb_state_e --bb_state
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode119
gcc/tree-tsan.c:119: struct bb_data_t
_t suffix is better removed. Same for other types with _t suffix.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode161
gcc/tree-tsan.c:161: tree __attribute__((weak))
Explain this.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode169
gcc/tree-tsan.c:169: extern __thread void **__tsan_shadow_stack; */
Need two white space before */. Same for other instances.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode182
gcc/tree-tsan.c:182:
Better use varpool_get_node interface.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode186
gcc/tree-tsan.c:186: TREE_STATIC (def) = 1;
Why mark TREE_STATIC (def) = 1? Should the variable be defined in tsan
library?
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode189
gcc/tree-tsan.c:189: DECL_TLS_MODEL (def) = decl_default_tls_model
(def);
Check if targetm.have_tls -- though for those target, tsan won't be
used.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode200
gcc/tree-tsan.c:200: {
Refactor the code so that it can be shared with the above one.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode228
gcc/tree-tsan.c:228: {
The name of the function is very confusing. Change it to
get_tsan_mop_handler_decl or something like that.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode251
gcc/tree-tsan.c:251: /* Adds new ignore definition to the global list */
Add documentation on function parameters (in upper case) such as TYPE
is the ignore type, and NAME is the name of the function to be ignored.
If there is return value, document it too.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode257
gcc/tree-tsan.c:257: desc = (struct tsan_ignore_desc_t*)xmalloc (sizeof
(*desc));
Use XCNEW to clear.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode264
gcc/tree-tsan.c:264: /* Checks as to whether identifier 'str' matches
template 'templ'.
Use STR instead of 'str'. 'templ' -- TEMPL.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode291
gcc/tree-tsan.c:291: if (spos == NULL)
Move the check up right after spos is computed.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode349
gcc/tree-tsan.c:349: printf (failed to open ignore file '%s'\n,
flag_tsan_ignore);
Use error (..)
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode360
gcc/tree-tsan.c:360: if (line [sz-1] == '\r' || line [sz-1] == '\n')
sz-1 -- sz - 1
Change other instances
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode391
gcc/tree-tsan.c:391: src_name =
expand_location(cfun-function_start_locus).file;
space before (
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode413
gcc/tree-tsan.c:413: static const char *
Missing documentation.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode443
gcc/tree-tsan.c:443: tree rtl_stack;
Do not use rtl_ prefix. Same for other instances.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode459
gcc/tree-tsan.c:459: s = NULL;
MODIFY_EXPR? directly use gimple_build_assign.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode725
gcc/tree-tsan.c:725:
This is wrong. SSA_NAME expr should be skipped.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode730
gcc/tree-tsan.c:730: {
remove {} Same for
