[RFR]: Field suppression list

[Jean Christophe Beyler]

Hi Arthur,

Sorry for the late reply :)

http://cr.openjdk.java.net/~aeubanks/tsanfieldsupp/webrev.00/src/hotspot/share/classfile/tsanIgnoreList.hpp.html

  33 // Lines that start with '#' are considered comment.

-> are considered comments.


  36 // Here are few examples.

-> Here are a few examples:


http://cr.openjdk.java.net/~aeubanks/tsanfieldsupp/webrev.00/src/hotspot/share/classfile/tsanIgnoreList.cpp.html

  - line 156 could be removed (extra empty line)

  - whereas line 175 could use an extra line :)

  - Is there a reason that we are using fgetc? I would just use fgets
and be done with all this logic handling lines...

  - FWIW, I think you should extract the whole file parsing into a
parse_from_file method, which would make the init : open a file, parse
it via parse_from_file, close it

  - parse_from_line:

     - I find this error prone:

        - I would really find a '#' and replace it with '\0' if found

        - then I would do a sscanf and check its return

        - I would add a few extra lines to better understand this code btw :)


  -   35     Exact,

     -> If we are going to do = 1, = 2, ... ; might as well do = 0


  41   FieldMatcher(const Symbol* class_name, const Symbol* field_name,
  42                Mode class_mode, Mode field_mode, FieldMatcher* next)

     -> Somehow class_name, class_mode, field_name, field_mode, next
seems like a better order when looking at it (to me :-))


Thanks,

Jc



On Tue, Jul 2, 2019 at 9:19 PM Arthur Eubanks <aeubanks at google.com> wrote:

> On Tue, Jul 2, 2019 at 4:31 PM Man Cao <manc at google.com> wrote:
>
> > Looks good. Some nits below, no need for another webrev.
> >
> > In classFileParser.cpp:
> > 37 #include "classfile/tsanIgnoreList.hpp"
> > This can be guarded by TSAN_ONLY.
> >
> We are using #if INCLUDE_TSAN rather than TSAN_ONLY for guarding #includes,
> doing that instead.
>
> >
> > In
> >
> http://cr.openjdk.java.net/~aeubanks/tsanfieldsupp/webrev.00/src/hotspot/share/classfile/tsanIgnoreList.cpp.html
> ,
> > some whitespace can be improved:
> >
> >   62  protected:
> >   63   const Symbol*        _class_name;
> >   64   const Symbol*        _field_name;
> >   65   Mode           _class_mode;
> >   66   Mode           _field_mode;
> >   67   FieldMatcher* _next;
> >
> > No need to have more than 1 space.
> >
> >  135     _exact_match = new FieldMatcher(class_symbol, field_symbol,
> class_mode,
> >  136                                    field_mode, _exact_match);
> >
> > Need to align second line.
> >
> >  156   char token[MAX_LINE_SIZE];
> >  157   int  pos = 0;
> >  158   int  c = fgetc(stream);
> >
> > No need to have more than 1 space.
> >
> > Done.
>
> > -Man
> >
> >
> > On Mon, Jul 1, 2019 at 3:59 PM Arthur Eubanks <aeubanks at google.com>
> wrote:
> >
> >> webrev:
> >> http://cr.openjdk.java.net/~aeubanks/tsanfieldsupp/webrev.00/index.html
> >>
> >> Add a new flag -XX:ThreadSanitizerIgnoreFile=path/to/ignorefile. The
> file
> >> is parsed for patterns like
> >>
> >> # To whitelist field myBaz in a class named com.foo.Bar
> >> com.foo.Bar myBaz
> >> # Every field with primitive type starting with my in that class:
> >> com.foo.Bar my*
> >> # And every primitive field in package com.foo:
> >> com.foo.* *
> >>
> >> and those fields are ignored in regards to TSAN.
> >>
> >
>


-- 

Thanks,
Jc