Add the following Clang plugins:...

Add the following Clang plugins:
PrintNamesAndSizes
  -Find top-level type definitions.  Print their kind (struct, enum, etc), typename, size, and location.
FindAffectedInterfaces
  -Given some typenames, find other types which are affected if that type changes.  This is specifically for determining which interfaces are affected by changing a struct or structs.

Also add a Makefile to build the plugins and a README describing how to run them.

Also add a python script that runs PrintNamesAndSizes with several different targets and generates compile-time checks in to the PPAPI source code to enforce that sizes are consistent, or if they change, we find out quickly.

This was broken off from a bigger CL:
http://codereview.chromium.org/5340003/
BUG=61004, 62983
TEST=None

See this CL for an example of what these plugins helped me do:
http://codereview.chromium.org/5674004



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69187

I'll address improving documentation per dspringer's recommendations in a soon-to-come CL.  I landed this to hopefully help Neb with a CL he's working on.

[brettw, 2010-12-12 21:54:15]

I don't understand clang, but this seems to make sense, so this is mostly a
rubber stamp with a could of style nits. LGTM

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/find_affec...
File ppapi/tests/clang/find_affected_interfaces.cc (right):

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/find_affec...
ppapi/tests/clang/find_affected_interfaces.cc:55: =
dyn_cast<clang::PointerType>(&type_to_check);
Style nit: = on previous line.

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/find_affec...
ppapi/tests/clang/find_affected_interfaces.cc:110:
dyn_cast<clang::TypedefDecl>(type_decl)) {
Style nit: indent to after the ( on the previous line.

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
File ppapi/tests/clang/print_names_and_sizes.cc (right):

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
ppapi/tests/clang/print_names_and_sizes.cc:112: =
tag->getASTContext().getTypeSizeInChars(type);
Style nit: = at the end of the previous line.

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
ppapi/tests/clang/print_names_and_sizes.cc:117:
dyn_cast<clang::TypedefDecl>(type_decl)) {
Style nit: indent to after the ( on the previous line.

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
ppapi/tests/clang/print_names_and_sizes.cc:144: }  // namespace
Style nit: blank line before this.

[dmichael(do not use this one), 2010-12-13 16:58:10]

Thanks, Brett!  I'll give Neb or Dave S a chance to look too before committing.

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/find_affec...
File ppapi/tests/clang/find_affected_interfaces.cc (right):

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/find_affec...
ppapi/tests/clang/find_affected_interfaces.cc:55: =
dyn_cast<clang::PointerType>(&type_to_check);
On 2010/12/12 21:54:15, brettw wrote:
> Style nit: = on previous line.
Done.

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/find_affec...
ppapi/tests/clang/find_affected_interfaces.cc:110:
dyn_cast<clang::TypedefDecl>(type_decl)) {
On 2010/12/12 21:54:15, brettw wrote:
> Style nit: indent to after the ( on the previous line.
Done.

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
File ppapi/tests/clang/print_names_and_sizes.cc (right):

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
ppapi/tests/clang/print_names_and_sizes.cc:112: =
tag->getASTContext().getTypeSizeInChars(type);
On 2010/12/12 21:54:15, brettw wrote:
> Style nit: = at the end of the previous line.
Done.

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
ppapi/tests/clang/print_names_and_sizes.cc:117:
dyn_cast<clang::TypedefDecl>(type_decl)) {
On 2010/12/12 21:54:15, brettw wrote:
> Style nit: indent to after the ( on the previous line.
Done.

http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
ppapi/tests/clang/print_names_and_sizes.cc:144: }  // namespace
On 2010/12/12 21:54:15, brettw wrote:
> Style nit: blank line before this.
Done.

[neb, 2010-12-13 18:23:23]

On 2010/12/13 16:58:10, dmichael wrote:
> Thanks, Brett!  I'll give Neb or Dave S a chance to look too before
committing.
> 
>
http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/find_affec...
> File ppapi/tests/clang/find_affected_interfaces.cc (right):
> 
>
http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/find_affec...
> ppapi/tests/clang/find_affected_interfaces.cc:55: =
> dyn_cast<clang::PointerType>(&type_to_check);
> On 2010/12/12 21:54:15, brettw wrote:
> > Style nit: = on previous line.
> Done.
> 
>
http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/find_affec...
> ppapi/tests/clang/find_affected_interfaces.cc:110:
> dyn_cast<clang::TypedefDecl>(type_decl)) {
> On 2010/12/12 21:54:15, brettw wrote:
> > Style nit: indent to after the ( on the previous line.
> Done.
> 
>
http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
> File ppapi/tests/clang/print_names_and_sizes.cc (right):
> 
>
http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
> ppapi/tests/clang/print_names_and_sizes.cc:112: =
> tag->getASTContext().getTypeSizeInChars(type);
> On 2010/12/12 21:54:15, brettw wrote:
> > Style nit: = at the end of the previous line.
> Done.
> 
>
http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
> ppapi/tests/clang/print_names_and_sizes.cc:117:
> dyn_cast<clang::TypedefDecl>(type_decl)) {
> On 2010/12/12 21:54:15, brettw wrote:
> > Style nit: indent to after the ( on the previous line.
> Done.
> 
>
http://codereview.chromium.org/5730003/diff/5001/ppapi/tests/clang/print_name...
> ppapi/tests/clang/print_names_and_sizes.cc:144: }  // namespace
> On 2010/12/12 21:54:15, brettw wrote:
> > Style nit: blank line before this.
> Done.

I don't know clang either, might be worth getting someone who does to chime in.
Otherwise, I see nothing objectionable - LGTM.

[dmichael(do not use this one), 2010-12-13 20:11:53]

+erg, for Clang expertise (if he has time).

[David Springer, 2010-12-13 21:23:08]

I tried to review this from the perspective of a potential user.  I asked for a
lot of comments and clarifications of the code - and also I would suggest adding
the usage to a wiki somewhere.  Instructions about "What do to if you trigger a
compile-time assertion" for instance.

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
File ppapi/generate_ppapi_size_checks.py (right):

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:8: have appropriate size checking.
I might be good to add this to a wiki somewhere (PPAPI design docs?) - as in
"what to do when I get a compile time assertion."

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:18: # The string that the PrintNamesAndSizes
plugin uses to indicate a type is or
"a type is or contains" -- ?

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:24: # 'unsigned long', we can add detection
of them to the plugin to automate this.
Uh-oh - this looks like a potential for human omission errors (which is what
your comment says).  Would it make sense to make the "add detection" part of the
comment a TODO(), and potentially file a bug?

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:48: """A class representing information
about a C++ type."""
Maybe add: "|info_string| has the following format: kind,name,size,...
  kind - the intrinsic type (is this correct?)
  name - the declared name
...etc."

Also a brief description of |target| would be most instructive.

Also, a brief description of the ivars for this class would be most helpful.

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:76: of the changes are applied until Apply
is called.  All line numbers are
Nit: spurious line-break.

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:88: """Make the patch delete the lines
[start_line, end_line)."""
Maybe state explicitly "delete lines starting with |start_line| up to but not
including |end_line|" instead of using the [) set notation.

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:247: def main(argv):
Suggest comment:
# See README file for example command-line invocation.

Also: some comment explaining the operational flow of this script.  E.g. "This
script runs clang over a set of source files.  Clang is run once for each
instruction set architecture, currently it runs in 32- and 64-bit modes.  The
source files are modified to include compile-time assertions based on type
sizes..."

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:319: # architecture.  These assertions go
immediately after the class definition.
It would be helpful for readers like me to see some sample output in the
comments here.  E.g.:
# struct PP_Var { inptr_t foo; }
# becomes:
# struct PP_Var { assert(the_assertion); intptr_t foo; };
(or whatever is appropriate).

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:333: c_source_root =
os.path.join(options.ppapi_root, "tests")
I am not sure what this is doing - it could use a comment, perhaps.  It appears
to be writing out tests that match the header patches you just generated in the
previous steps.

http://codereview.chromium.org/5730003/diff/10001/ppapi/generate_ppapi_size_c...
ppapi/generate_ppapi_size_checks.py:333: c_source_root =
os.path.join(options.ppapi_root, "tests")
A comment might be helpful here: I am not certain what's going on.  It looks
like this is writing out some tests that match the file patches made above.  Is
that right?

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/README
File ppapi/tests/clang/README (right):

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/README#ne...
ppapi/tests/clang/README:1: This is a directory for Clang plugins that are
designed to do analysis and/or manipulation of PPAPI code.  Clang is an
open-source C front-end that allows you to parse C, C++, or Objective-C code in
to an abstract syntax tree (or AST) for processing.
Nit: I don't know of any Style Guide for READMEs, but would you consider making
lines 80-cols?

Mention that you need a chromium checkout to run this stuff (lift the last
comment up here)?

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/README#ne...
ppapi/tests/clang/README:17: PrintNamesAndSizes : print_names_and_sizes.cc
Suggestion: offset the individual plugin description sections with indentation.
E.g.:
Plugins:
  PrintNamesAndSizes:
  ...

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/README#ne...
ppapi/tests/clang/README:18: Print information about all top-level type
definitions.  You probably won't need to run it by itself;  instead see
generate_ppapi_size_checks.py, which uses the plugin.
What information do I get?  Example output?

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/README#ne...
ppapi/tests/clang/README:22: python generate_ppapi_size_checks.py --help
Example output?  Why would I use this particular plugin?  Do I need to run it
when I trigger the compile-time assertions in the PPAPI structs?

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/README#ne...
ppapi/tests/clang/README:32: (This assumes that clang is in your path and you
are running the plugin from the ppapi subdirectory in a chrome checkout).
"in a chrome checkout" - this fact is important enough that it deserves top
billing.  I realize that devs probably have a chrome checkout already (otherwise
how are they reading this file?), but someone browsing the source on the web
might need to know this fact.

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/find_affe...
File ppapi/tests/clang/find_affected_interfaces.cc (right):

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/find_affe...
ppapi/tests/clang/find_affected_interfaces.cc:7: // C-level interfaces are
affected if one or more PPAPI C structs are changed.
This comment seems a little terse to me - perhaps add an example, as in:
"If you change the size of PP_Var, then these decls will be flagged: bool
Foo(PP_Var* var)."

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/find_affe...
ppapi/tests/clang/find_affected_interfaces.cc:42: bool IsAffected(const
clang::Type& type_to_check) {
Please comment this method.  It appears to be the business end of your code, so
understanding how it works would be most beneficial.

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/find_affe...
ppapi/tests/clang/find_affected_interfaces.cc:53: if
(type_to_check.isPointerType()) {
I am not able to get a clear picture in my head of how this relates to the input
code stream.  Maybe an example of input that triggers this clause?  E.g.
"Process input code like this: PP_Var* var_ptr" or whatever is the correct
thing.

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/find_affe...
ppapi/tests/clang/find_affected_interfaces.cc:55:
dyn_cast<clang::PointerType>(&type_to_check);
Maybe a quick comment on dyn_cast vs. dynamic_cast.  E.g. "use dyn_cast because
clang recommends it, and it does not require RTTI" or whatever.  "Copied from
the clang example" is perfectly acceptable, just something to make this usage
less magical.

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/find_affe...
ppapi/tests/clang/find_affected_interfaces.cc:59: const
clang::FunctionProtoType* func_type =
An input stream example that triggers this would be most helpful.

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/find_affe...
ppapi/tests/clang/find_affected_interfaces.cc:73: // For records (unions,
structs), recurse to the fields.
Input stream example?

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/find_affe...
ppapi/tests/clang/find_affected_interfaces.cc:91: // it.
But the above IsAffected() code processes function decls and things seemingly
other than simple typedefs.  Could you expand on this comment a little to
explain how this method uses IsAffected()?

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/print_nam...
File ppapi/tests/clang/print_names_and_sizes.cc (right):

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/print_nam...
ppapi/tests/clang/print_names_and_sizes.cc:36: // - source_file:  The source
file in which the type is defined.
Is this a full POSIX path?  An OS-specific path? Or just the leaf file name?

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/print_nam...
ppapi/tests/clang/print_names_and_sizes.cc:37: // - first_line:  The first line
of the definition (counting from 0).
"first line number" - just to be clear that you want an int value here.  Not,
for instance, the text of the first line.

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/print_nam...
ppapi/tests/clang/print_names_and_sizes.cc:50: // Return true iff the type is a
pointer or contains a pointer.  This is
"or contains a pointer" - as in:
struct PP_Var {
  int32_t* ptr;
};
?

http://codereview.chromium.org/5730003/diff/10001/ppapi/tests/clang/print_nam...
ppapi/tests/clang/print_names_and_sizes.cc:60: // contain is or has a pointer,
then we have one too.
"...contain is or has a pointer..." -- ?

[Elliot Glaysher, 2010-12-13 21:56:43]

I didn't do an in depth check, but a brief passover the code found
nothing objectionable. OK.

On Mon, Dec 13, 2010 at 12:11 PM,  <dmichael@google.com> wrote:
> +erg, for Clang expertise (if he has time).
>
> http://codereview.chromium.org/5730003/
>