This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
- From: davidxl at google dot com
- To: dvyukov at google dot com, gcc-patches at gcc dot gnu dot org, dnovillo at google dot com
- Cc: kcc at google dot com, reply at codereview dot appspotmail dot com
- Date: Mon, 31 Oct 2011 06:08:35 +0000
- Subject: Re: [google] ThreadSanitizer instrumentation pass (issue 5303083)
- Reply-to: dvyukov at google dot com, gcc-patches at gcc dot gnu dot org, davidxl at google dot com, dnovillo at google dot com, kcc at google dot com, reply at codereview dot appspotmail dot com
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 other one liners. Many conditions should be merged
using ||
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode751
gcc/tree-tsan.c:751: && TREE_STATIC (expr) == 0)
If you want to skip local variables, check DECL_EXTERNAL attribute.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode783
gcc/tree-tsan.c:783: tree field = expr->exp.operands [1];
Do not directly reference tree's member like this. Use TREE_OPERAND
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode786
gcc/tree-tsan.c:786: fld_off =
field->field_decl.bit_offset->int_cst.int_cst.low;
Do not access tree like this. Use DECL_FIELD_OFFSET etc.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode844
gcc/tree-tsan.c:844: }
Remove code handling arguments. THey should not have mem refs.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode854
gcc/tree-tsan.c:854:
Remove lhs handling -- no need.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode877
gcc/tree-tsan.c:877: }
Remove handling of GIMPLE_BIND
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode1003
gcc/tree-tsan.c:1003: {
For topo order traversal, use inverted_post_order_compute and then
handling bb s in that order. This will simplify the code a lot.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode1141
gcc/tree-tsan.c:1141: {
Better way is to walk through all predecessors of EXIT_BLOCK_PTR.
http://codereview.appspot.com/5303083/diff/1/gcc/tree-tsan.c#newcode1167
gcc/tree-tsan.c:1167: return 0;
No need for the above.
http://codereview.appspot.com/5303083/