Check for unstable types in IPC messages.

Check for unstable types in IPC messages.

We want IPC messages to be stable so that 32-bit and 64-bit processes
can talk to each other. This change adds a check to find-bad-constructs
Clang plugin to detect usage of the following unstable types in IPC
messages:

1. Types: long / unsigned long (but not typedefs to)
2. Typedefs: intmax_t, uintmax_t, intptr_t, uintptr_t, wint_t, size_t,
   rsize_t, ssize_t, ptrdiff_t, dev_t, off_t, clock_t, time_t, suseconds_t
   (including typedefs to)
3. Any template referencing the above (e.g. std::vector<size_t>)

BUG=581409
Committed: https://crrev.com/2bc894640e0423cad921aed8d234adc375999771
Cr-Commit-Position: refs/heads/master@{#385469}

[Dmitry Skiba, 2016-03-18 17:01:33]

dskiba@google.com changed reviewers:
+ dcheng@chromium.org, jam@chromium.org, thakis@chromium.org

[Dmitry Skiba, 2016-03-18 17:01:33]

Fixed version of (reverted) CL from https://codereview.chromium.org/1665363002

Guys, I would appreciate of you try this change on Windows. I've received my
Windows laptop, but hasn't set it up yet.

[jam, 2016-03-18 18:09:46]

On 2016/03/18 17:01:33, Dmitry Skiba wrote:
> Fixed version of (reverted) CL from https://codereview.chromium.org/1665363002

is patchset 1 the reverted change, and patchset 2 is fixes?

> 
> Guys, I would appreciate of you try this change on Windows. I've received my
> Windows laptop, but hasn't set it up yet.

not sure who this is aimed at, and what you want folks to test.

building chrome on a windows laptop is going to be very painful. if you want to
build windows, i suggest getting a z840.

[dcheng, 2016-03-19 00:11:32]

On 2016/03/18 at 18:09:46, jam wrote:
> On 2016/03/18 17:01:33, Dmitry Skiba wrote:
> > Fixed version of (reverted) CL from
https://codereview.chromium.org/1665363002
> 
> is patchset 1 the reverted change, and patchset 2 is fixes?
> 
> > 
> > Guys, I would appreciate of you try this change on Windows. I've received my
> > Windows laptop, but hasn't set it up yet.
> 
> not sure who this is aimed at, and what you want folks to test.
> 
> building chrome on a windows laptop is going to be very painful. if you want
to build windows, i suggest getting a z840.

I'm OOO until Tuesday. If I can convince RDP to work, I'll patch it in to my
Windows machine and see if the plugin tests pass.

[Dmitry Skiba, 2016-03-21 17:49:36]

On 2016/03/18 18:09:46, jam wrote:
> On 2016/03/18 17:01:33, Dmitry Skiba wrote:
> > Fixed version of (reverted) CL from
https://codereview.chromium.org/1665363002
> 
> is patchset 1 the reverted change, and patchset 2 is fixes?
> 
> > 
> > Guys, I would appreciate of you try this change on Windows. I've received my
> > Windows laptop, but hasn't set it up yet.
> 
> not sure who this is aimed at, and what you want folks to test.
> 
> building chrome on a windows laptop is going to be very painful. if you want
to
> build windows, i suggest getting a z840.

Yes, patchset 2 is the fix. Sorry I wasn't clear, I wanted Clang guys to try my
change, because it was reverted after Clang crashed on one of the Windows bots.

I won't be building Chrome on my Windows laptop, just Clang and
find-bad-constructs with my patches.

[jam, 2016-03-24 21:56:31]

On 2016/03/21 17:49:36, Dmitry Skiba wrote:
> On 2016/03/18 18:09:46, jam wrote:
> > On 2016/03/18 17:01:33, Dmitry Skiba wrote:
> > > Fixed version of (reverted) CL from
> https://codereview.chromium.org/1665363002
> > 
> > is patchset 1 the reverted change, and patchset 2 is fixes?
> > 
> > > 
> > > Guys, I would appreciate of you try this change on Windows. I've received
my
> > > Windows laptop, but hasn't set it up yet.
> > 
> > not sure who this is aimed at, and what you want folks to test.
> > 
> > building chrome on a windows laptop is going to be very painful. if you want
> to
> > build windows, i suggest getting a z840.
> 
> Yes, patchset 2 is the fix. Sorry I wasn't clear, I wanted Clang guys to try
my
> change, because it was reverted after Clang crashed on one of the Windows
bots.
> 
> I won't be building Chrome on my Windows laptop, just Clang and
> find-bad-constructs with my patches.

still lgtm for ipc/

[Dmitry Skiba, 2016-03-25 16:48:24]

Finally checked in Windows - tests pass (when ran manually), no crashes.

[Dmitry Skiba, 2016-03-30 18:18:32]

Nico, Daniel - please review?

[dcheng, 2016-03-30 18:29:22]

https://codereview.chromium.org/1810243002/diff/20001/tools/clang/plugins/Fin...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1810243002/diff/20001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:940: // template trace method
bodies.
Fix this comment. Also, how come we don't need to ignore system headers like
https://codereview.chromium.org/1135333007 did?

[Dmitry Skiba, 2016-03-30 23:08:54]

On 2016/03/30 18:29:22, dcheng wrote:
>
https://codereview.chromium.org/1810243002/diff/20001/tools/clang/plugins/Fin...
> File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):
> 
>
https://codereview.chromium.org/1810243002/diff/20001/tools/clang/plugins/Fin...
> tools/clang/plugins/FindBadConstructsConsumer.cpp:940: // template trace
method
> bodies.
> Fix this comment. Also, how come we don't need to ignore system headers like
> https://codereview.chromium.org/1135333007 did?

Done. System headers check was removed by mistake (I just really like to delete
code).

[dcheng, 2016-04-04 06:21:44]

https://codereview.chromium.org/1810243002/diff/60001/tools/clang/plugins/Fin...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1810243002/diff/60001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:100: bool
VisitFunctionDecl(FunctionDecl* function_decl) {
Nico, do you recall your reason for adding a new RecursiveASTVisitor for this in
https://codereview.chromium.org/1135333007? It seems like it could just be on
the top-level RecursiveASTVisitor instead.

https://codereview.chromium.org/1810243002/diff/60001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:930: void
FindBadConstructsConsumer::ParseFunctionTemplates(
Can you add a comment about where this snippet comes from? Just reference Nico's
CL doing something similar for the Oilpan plugin.

[Nico, 2016-04-05 18:51:45]

https://codereview.chromium.org/1810243002/diff/60001/tools/clang/plugins/Fin...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1810243002/diff/60001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:100: bool
VisitFunctionDecl(FunctionDecl* function_decl) {
On 2016/04/04 06:21:43, dcheng wrote:
> Nico, do you recall your reason for adding a new RecursiveASTVisitor for this
in
> https://codereview.chromium.org/1135333007 It seems like it could just be on
> the top-level RecursiveASTVisitor instead.

I don't. It seemed like a standalone thing, so I made it a standalone thing. Are
there advantages to putting it in the toplevel visitor?

[dcheng, 2016-04-05 18:58:47]

https://codereview.chromium.org/1810243002/diff/60001/tools/clang/plugins/Fin...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1810243002/diff/60001/tools/clang/plugins/Fin...
tools/clang/plugins/FindBadConstructsConsumer.cpp:100: bool
VisitFunctionDecl(FunctionDecl* function_decl) {
On 2016/04/05 at 18:51:45, Nico (hiding until Fri) wrote:
> On 2016/04/04 06:21:43, dcheng wrote:
> > Nico, do you recall your reason for adding a new RecursiveASTVisitor for
this in
> > https://codereview.chromium.org/1135333007 It seems like it could just be on
> > the top-level RecursiveASTVisitor instead.
> 
> I don't. It seemed like a standalone thing, so I made it a standalone thing.
Are there advantages to putting it in the toplevel visitor?

Mostly just a matter of how many times we end up re-traversing the AST. I expect
compile-time cost to dominate, but it would be nice to confirm that.

[Dmitry Skiba, 2016-04-05 22:13:51]

On 2016/04/05 18:58:47, dcheng wrote:
>
https://codereview.chromium.org/1810243002/diff/60001/tools/clang/plugins/Fin...
> File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):
> 
>
https://codereview.chromium.org/1810243002/diff/60001/tools/clang/plugins/Fin...
> tools/clang/plugins/FindBadConstructsConsumer.cpp:100: bool
> VisitFunctionDecl(FunctionDecl* function_decl) {
> On 2016/04/05 at 18:51:45, Nico (hiding until Fri) wrote:
> > On 2016/04/04 06:21:43, dcheng wrote:
> > > Nico, do you recall your reason for adding a new RecursiveASTVisitor for
> this in
> > > https://codereview.chromium.org/1135333007 It seems like it could just be
on
> > > the top-level RecursiveASTVisitor instead.
> > 
> > I don't. It seemed like a standalone thing, so I made it a standalone thing.
> Are there advantages to putting it in the toplevel visitor?
> 
> Mostly just a matter of how many times we end up re-traversing the AST. I
expect
> compile-time cost to dominate, but it would be nice to confirm that.

But we call ParseFunctionTemplates() only if check-ipc is specified, and it
won't be specified on Windows. The whole change is just for completeness sake,
to pass IPC tests on Windows.

[dcheng, 2016-04-06 00:20:52]

LGTM

[somtuy41, 2016-04-06 04:22:03]

somtuy41@gmail.com changed reviewers:
+ somtuy41@gmail.com

[Dmitry Skiba, 2016-04-06 05:38:17]

The CQ bit was checked by dskiba@google.com

[Dmitry Skiba, 2016-04-06 05:38:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1810243002/#ps100001
(title: "Add comment")

[commit-bot: I haz the power, 2016-04-06 05:38:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1810243002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1810243002/100001

[commit-bot: I haz the power, 2016-04-06 06:48:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-06 06:48:43]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dmitry Skiba, 2016-04-06 15:11:56]

The CQ bit was checked by dskiba@google.com

[commit-bot: I haz the power, 2016-04-06 15:12:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1810243002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1810243002/100001

[commit-bot: I haz the power, 2016-04-06 15:51:16]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-06 15:53:08]

Description was changed from

==========
Check for unstable types in IPC messages.

We want IPC messages to be stable so that 32-bit and 64-bit processes
can talk to each other. This change adds a check to find-bad-constructs
Clang plugin to detect usage of the following unstable types in IPC
messages:

1. Types: long / unsigned long (but not typedefs to)
2. Typedefs: intmax_t, uintmax_t, intptr_t, uintptr_t, wint_t, size_t,
   rsize_t, ssize_t, ptrdiff_t, dev_t, off_t, clock_t, time_t, suseconds_t
   (including typedefs to)
3. Any template referencing the above (e.g. std::vector<size_t>)

BUG=581409
==========

to

==========
Check for unstable types in IPC messages.

We want IPC messages to be stable so that 32-bit and 64-bit processes
can talk to each other. This change adds a check to find-bad-constructs
Clang plugin to detect usage of the following unstable types in IPC
messages:

1. Types: long / unsigned long (but not typedefs to)
2. Typedefs: intmax_t, uintmax_t, intptr_t, uintptr_t, wint_t, size_t,
   rsize_t, ssize_t, ptrdiff_t, dev_t, off_t, clock_t, time_t, suseconds_t
   (including typedefs to)
3. Any template referencing the above (e.g. std::vector<size_t>)

BUG=581409

Committed: https://crrev.com/2bc894640e0423cad921aed8d234adc375999771
Cr-Commit-Position: refs/heads/master@{#385469}
==========

[commit-bot: I haz the power, 2016-04-06 15:53:10]

Patchset 6 (id:??) landed as
https://crrev.com/2bc894640e0423cad921aed8d234adc375999771
Cr-Commit-Position: refs/heads/master@{#385469}

[somtuy41, 2016-07-19 09:01:10]

เมื่อ 6 เม.ย. 2016 22:53 "commit-bot@chromium.org via
codereview.chromium.org" <reply@chromiumcodereview-hr.appspotmail.com>
เขียนว่า:
>
> Patchset 6 (id:??) landed as
> https://crrev.com/2bc894640e0423cad921aed8d234adc375999771
> Cr-Commit-Position: refs/heads/master@{#385469}
>
> https://codereview.chromium.org/1810243002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.