This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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 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/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]