Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1665363002:
Clang plugin to check that unstable types are not used in IPC. (Closed)
|
Created:
4 years, 10 months ago by Dmitry Skiba Modified:
4 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClang plugin to check that unstable types are not used in IPC.
We want IPC messages to be stable so that 32-bit and 64-bit processes
can talk to each other. This plugin blacklists the following types that
are known to be unstable:
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)
Blacklisted types are not allowed in IPC::WriteParam() invocations or
IPC::CheckedTuple<> specializations.
BUG=581409
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/a817c371a7a33cdf9282b711dafbdcee83b788e4
Cr-Commit-Position: refs/heads/master@{#380151}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Plug IPC check into FindBadConstructs #Patch Set 3 : Enable for GYP builds #Patch Set 4 : Reworked plugin #Patch Set 5 : rebased #Patch Set 6 : Android fixes; comments #Patch Set 7 : Blacklist types instead #
Total comments: 2
Patch Set 8 : Check specializations in WriteParam() too; revert ppapi change" #Patch Set 9 : Require with-ast-visitor flag (preparation for rebase) #Patch Set 10 : rebased #Patch Set 11 : Fix GPU IPC #Patch Set 12 : Remove extra RAV #Patch Set 13 : Fix typos #
Total comments: 32
Patch Set 14 : Address comments (1/2) #Patch Set 15 : Address comments (2/2, CheckType and friends -> CheckIPCVisitor) #Patch Set 16 : Don't check WriteParam() on dependent types #Patch Set 17 : rebased #Patch Set 18 : Use SmallVector; revert build script changes #
Total comments: 24
Patch Set 19 : Address comments #Patch Set 20 : Use IgnoreImplicit() #Patch Set 21 : rebase #Patch Set 22 : rebase to ToT #
Total comments: 1
Patch Set 23 : Support delayed-template-parsing; remove flaky test; don't visit instantiations #Messages
Total messages: 81 (16 generated)
Description was changed from ========== Clang plugin to check size_t usage in IPC. Plugin checks various ways size_t can creep in an IPC message and errors on them. BUG=581409 ========== to ========== Clang plugin to check size_t usage in IPC. Plugin checks various ways size_t can creep in an IPC message and errors on them. Specifically the plugin checks that: 1. IPC::ParamTuple<> is not specialized with size_t. 2. IPC::WriteParam() is not called on size_t argument. 3. IPC::WriteParam() is not called on __SIZE_TYPE__ argument inside implicit template instantiation (where we don't know whether it's size_t or not), unless it's IPC::ParamTraits<IPC::ParamTuple> instantiation. See bug for more info. BUG=581409 ==========
dskiba@google.com changed reviewers: + thakis@chromium.org
Please note that I'm in the process of splitting the plugin into .h / .cpp pair and integrating it into FindBadConstructsAction. Suggestions on how to do that are welcome because I need access to ASTContext, but FindBadConstructs has this lovely with-ast-visitor flag, which is not enabled by default. For now I think I'll call my visitor either inside PluginConsumer::HandleTranslationUnit (if flag is on), or inside FindBadConstructsConsumer::HandleTranslationUnit (if flag is off).
thakis@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng Can you please check that this doesn't regress compile time perf (build with and without this 2-3 times). https://codereview.chromium.org/751233002/#msg15 suggests that it shouldn't be a problem.
On 2016/02/05 19:29:57, Nico wrote: > +dcheng > > Can you please check that this doesn't regress compile time perf (build with and > without this 2-3 times). https://codereview.chromium.org/751233002/#msg15 > suggests that it shouldn't be a problem. Sure, let me just integrate it properly. In the meantime, can you please glance over it - there might be general issues or misuses, as it's my first plugin.
On 2016/02/05 at 19:26:30, dskiba wrote: > Please note that I'm in the process of splitting the plugin into .h / .cpp pair and integrating it into FindBadConstructsAction. > > Suggestions on how to do that are welcome because I need access to ASTContext, but FindBadConstructs has this lovely with-ast-visitor flag, which is not enabled by default. For now I think I'll call my visitor either inside PluginConsumer::HandleTranslationUnit (if flag is on), or inside FindBadConstructsConsumer::HandleTranslationUnit (if flag is off). Yeah... I haven't had time to figure out why RecursiveASTVisitor is much more aggressive about warnings than the non-RecursiveASTVisitor version. I'll see if I can poke at this again, having this flag generally make me sad. Also, what about things like off_t and ssize_t? (Also, some random drive by nits. Sorry!) https://codereview.chromium.org/1665363002/diff/1/tools/clang/plugins/CheckIP... File tools/clang/plugins/CheckIPCAction.cpp (right): https://codereview.chromium.org/1665363002/diff/1/tools/clang/plugins/CheckIP... tools/clang/plugins/CheckIPCAction.cpp:41: QualType desugaredType = type.getSingleStepDesugaredType(context); Nit: desugared_type https://codereview.chromium.org/1665363002/diff/1/tools/clang/plugins/CheckIP... tools/clang/plugins/CheckIPCAction.cpp:61: SizeTExprFinder(ASTContext& context): context_(context), found_(nullptr) {} explicit https://codereview.chromium.org/1665363002/diff/1/tools/clang/plugins/CheckIP... tools/clang/plugins/CheckIPCAction.cpp:170: Decl* getParentDecl() const { GetParentDecl()?
Please check new version - IPC check is now behind a flag in find-bad-constructs plugin (instead of being of it's own). I also addressed Daniel's nits. Two things remain: 1. Check performance. 2. Think about off_t, etc. I think I'll just use of "int", "unsigned int", "long", "unsigned long" and all typedefs which are not (or not based on) "int32_t", "uint32_t", "int64_t" and "uint64_t".
On 2016/02/05 23:27:21, Dmitry Skiba wrote: > Please check new version - IPC check is now behind a flag in find-bad-constructs > plugin (instead of being of it's own). I also addressed Daniel's nits. Two > things remain: > > 1. Check performance. > 2. Think about off_t, etc. I think I'll just use of "int", "unsigned int", > "long", "unsigned long" and all typedefs which are not (or not based on) > "int32_t", "uint32_t", "int64_t" and "uint64_t". * "I'll just prohibit use of"
On 2016/02/05 23:28:16, Dmitry Skiba wrote: > On 2016/02/05 23:27:21, Dmitry Skiba wrote: > > Please check new version - IPC check is now behind a flag in > find-bad-constructs > > plugin (instead of being of it's own). I also addressed Daniel's nits. Two > > things remain: > > > > 1. Check performance. > > 2. Think about off_t, etc. I think I'll just use of "int", "unsigned int", > > "long", "unsigned long" and all typedefs which are not (or not based on) > > "int32_t", "uint32_t", "int64_t" and "uint64_t". > > * "I'll just prohibit use of" Some performance numbers. 1. When running all tests (without ipc.cpp) the difference is not detectable. In fact version without my plugin (Visit() calls commented out in both FindBadConstructsAction / Consumer) is slower :) WITH: (0.373 + 0.379 + 0.373 + 0.383 + 0.373) / 5 = 0.3762 WITHOUT: (0.382 + 0.398 + 0.398 + 0.389 + 0.388) / 5 = 0.391 2. So I created special version of ipc.cpp where I macro-generate 10000 copies of main() without failure cases (4 OK cases). With that I was able to detect a difference: WITH: (3.928 + 3.960 + 3.986 + 3.946 + 3.914) / 5 = 3.9468 WITHOUT: (3.943 + 3.901 + 3.914 + 3.983 + 3.863) / 5 = 3.9208 So it looks like the performance is fine?
On 2016/02/09 at 09:18:29, dskiba wrote: > On 2016/02/05 23:28:16, Dmitry Skiba wrote: > > On 2016/02/05 23:27:21, Dmitry Skiba wrote: > > > Please check new version - IPC check is now behind a flag in > > find-bad-constructs > > > plugin (instead of being of it's own). I also addressed Daniel's nits. Two > > > things remain: > > > > > > 1. Check performance. > > > 2. Think about off_t, etc. I think I'll just use of "int", "unsigned int", > > > "long", "unsigned long" and all typedefs which are not (or not based on) > > > "int32_t", "uint32_t", "int64_t" and "uint64_t". > > > > * "I'll just prohibit use of" > > Some performance numbers. > > 1. When running all tests (without ipc.cpp) the difference is not detectable. In fact version without my plugin (Visit() calls commented out in both FindBadConstructsAction / Consumer) is slower :) > WITH: (0.373 + 0.379 + 0.373 + 0.383 + 0.373) / 5 = 0.3762 > WITHOUT: (0.382 + 0.398 + 0.398 + 0.389 + 0.388) / 5 = 0.391 > > 2. So I created special version of ipc.cpp where I macro-generate 10000 copies of main() without failure cases (4 OK cases). With that I was able to detect a difference: > WITH: (3.928 + 3.960 + 3.986 + 3.946 + 3.914) / 5 = 3.9468 > WITHOUT: (3.943 + 3.901 + 3.914 + 3.983 + 3.863) / 5 = 3.9208 > > So it looks like the performance is fine? It might be useful to time a build across Chrome with the new plugin (which implies use_goma=no, unfortunately) to see how build time in Chrome is or isn't affected.
On 2016/02/09 18:12:22, dcheng wrote: > On 2016/02/09 at 09:18:29, dskiba wrote: > > On 2016/02/05 23:28:16, Dmitry Skiba wrote: > > > On 2016/02/05 23:27:21, Dmitry Skiba wrote: > > > > Please check new version - IPC check is now behind a flag in > > > find-bad-constructs > > > > plugin (instead of being of it's own). I also addressed Daniel's nits. Two > > > > things remain: > > > > > > > > 1. Check performance. > > > > 2. Think about off_t, etc. I think I'll just use of "int", "unsigned int", > > > > "long", "unsigned long" and all typedefs which are not (or not based on) > > > > "int32_t", "uint32_t", "int64_t" and "uint64_t". > > > > > > * "I'll just prohibit use of" > > > > Some performance numbers. > > > > 1. When running all tests (without ipc.cpp) the difference is not detectable. > In fact version without my plugin (Visit() calls commented out in both > FindBadConstructsAction / Consumer) is slower :) > > WITH: (0.373 + 0.379 + 0.373 + 0.383 + 0.373) / 5 = 0.3762 > > WITHOUT: (0.382 + 0.398 + 0.398 + 0.389 + 0.388) / 5 = 0.391 > > > > 2. So I created special version of ipc.cpp where I macro-generate 10000 copies > of main() without failure cases (4 OK cases). With that I was able to detect a > difference: > > WITH: (3.928 + 3.960 + 3.986 + 3.946 + 3.914) / 5 = 3.9468 > > WITHOUT: (3.943 + 3.901 + 3.914 + 3.983 + 3.863) / 5 = 3.9208 > > > > So it looks like the performance is fine? > > It might be useful to time a build across Chrome with the new plugin (which > implies use_goma=no, unfortunately) to see how build time in Chrome is or isn't > affected. OK, I'll check it. Any other comments? How about way I integrated IPC checker into both FindBadConstructsAction / Consumer so that its run regardless of with-ast-visitor flag?
Description was changed from ========== Clang plugin to check size_t usage in IPC. Plugin checks various ways size_t can creep in an IPC message and errors on them. Specifically the plugin checks that: 1. IPC::ParamTuple<> is not specialized with size_t. 2. IPC::WriteParam() is not called on size_t argument. 3. IPC::WriteParam() is not called on __SIZE_TYPE__ argument inside implicit template instantiation (where we don't know whether it's size_t or not), unless it's IPC::ParamTraits<IPC::ParamTuple> instantiation. See bug for more info. BUG=581409 ========== to ========== Clang plugin to check that unstable types are not used in IPC. We want IPC messages to be stable so that 32-bit and 64-bit processes can talk to each other. However, there are types that are not stable in that regard, for example long or size_t. We can't just ban long because there are legitimate typedefs that use it (int64_t). On the other hand, unstable typedefs use long too (size_t). Since typedefs that we care about are defined to either int or long, we enforce the following rules by the plugin: 1. Only check types equivalent to (unsigned) int and (unsigned) long 2. Allow (unsigned) int when it's used as is (not via typedef) 3. Only allow int32_t / uint32_t and int64_t / uint64_t typedefs to (unsigned) int or (unsigned) long Types are checked at IPC::WriteParam() invocations, so other checks enforced by the plugin ensure that IPC::WriteParam() is not used in a way that prevents checking it. BUG=581409 ==========
dcheng@chromium.org changed reviewers: + vmpstr@chromium.org
+vmpstr, who was poking at some of the RecursiveASTVisitor stuff last week and thinking about ways to switch the plugin over to use RecursiveASTVisitor completely. (Also, see https://crbug.com/436357)
Description was changed from ========== Clang plugin to check that unstable types are not used in IPC. We want IPC messages to be stable so that 32-bit and 64-bit processes can talk to each other. However, there are types that are not stable in that regard, for example long or size_t. We can't just ban long because there are legitimate typedefs that use it (int64_t). On the other hand, unstable typedefs use long too (size_t). Since typedefs that we care about are defined to either int or long, we enforce the following rules by the plugin: 1. Only check types equivalent to (unsigned) int and (unsigned) long 2. Allow (unsigned) int when it's used as is (not via typedef) 3. Only allow int32_t / uint32_t and int64_t / uint64_t typedefs to (unsigned) int or (unsigned) long Types are checked at IPC::WriteParam() invocations, so other checks enforced by the plugin ensure that IPC::WriteParam() is not used in a way that prevents checking it. BUG=581409 ========== to ========== Clang plugin to check that unstable types are not used in IPC. We want IPC messages to be stable so that 32-bit and 64-bit processes can talk to each other. However, there are types that are not stable, for example long or size_t. We can't just ban long because there are legitimate typedefs that use it (int64_t). On the other hand, unstable typedefs use long too (intptr_t). Since typedefs that we care about are defined to either int or long, plugin enforces the following rules: 1. Only check types equivalent to (unsigned) int and (unsigned) long 2. Allow (unsigned) int when it's used as is (not via typedef) 3. Only allow int32_t / uint32_t and int64_t / uint64_t typedefs to (unsigned) int or (unsigned) long Types are enforced at IPC::WriteParam() invocations and at IPC::CheckedTuple<> specializations. BUG=581409 ==========
jam@chromium.org changed reviewers: + jam@chromium.org
I left comments on the bug, summary is: 1) remove the int restriction; this isn't needed since int is the same size across 32/64. this removes the need for IPC_STRUCT_TRAITS_MEMBER_AS and the changes in chrome/content (other than protobuf_message_param_traits.h which fixed a size_t) 2) remove long restriction since we have compile time checks for it on Android 32. That way the plugin can be enabled on Linux as well for local dev builds.
We now use RecursiveASTVisitor by default: https://codereview.chromium.org/1703713002 So I think you can simplify this somewhat now.
On 2016/02/18 07:12:37, dcheng wrote: > We now use RecursiveASTVisitor by default: > https://codereview.chromium.org/1703713002 > > So I think you can simplify this somewhat now. Cool, I'll update today.
Description was changed from ========== Clang plugin to check that unstable types are not used in IPC. We want IPC messages to be stable so that 32-bit and 64-bit processes can talk to each other. However, there are types that are not stable, for example long or size_t. We can't just ban long because there are legitimate typedefs that use it (int64_t). On the other hand, unstable typedefs use long too (intptr_t). Since typedefs that we care about are defined to either int or long, plugin enforces the following rules: 1. Only check types equivalent to (unsigned) int and (unsigned) long 2. Allow (unsigned) int when it's used as is (not via typedef) 3. Only allow int32_t / uint32_t and int64_t / uint64_t typedefs to (unsigned) int or (unsigned) long Types are enforced at IPC::WriteParam() invocations and at IPC::CheckedTuple<> specializations. BUG=581409 ========== to ========== Clang plugin to check that unstable types are not used in IPC. We want IPC messages to be stable so that 32-bit and 64-bit processes can talk to each other. This plugin blacklists the following types that are known to be unstable: 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) Blacklisted types are not allowed in IPC::WriteParam() invocations or IPC::CheckedTuple<> specializations. BUG=581409 ==========
https://codereview.chromium.org/1665363002/diff/120001/ppapi/proxy/ppapi_mess... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/1665363002/diff/120001/ppapi/proxy/ppapi_mess... ppapi/proxy/ppapi_messages.h:493: int32_t /* renderer_pid */, no need to change right?
On 2016/02/18 21:04:49, jam wrote: > https://codereview.chromium.org/1665363002/diff/120001/ppapi/proxy/ppapi_mess... > File ppapi/proxy/ppapi_messages.h (right): > > https://codereview.chromium.org/1665363002/diff/120001/ppapi/proxy/ppapi_mess... > ppapi/proxy/ppapi_messages.h:493: int32_t /* renderer_pid */, > no need to change right? Right, I'll remove it in next update.
Performance numbers from building 32-bit chrome_apk target in Release: With plugin: real 47m20.095s user 505m55.766s sys 18m23.780s Without plugin: real 46m25.170s user 495m29.953s sys 18m1.625s
On 2016/02/18 23:14:30, Dmitry Skiba wrote: > On 2016/02/18 21:04:49, jam wrote: > > > https://codereview.chromium.org/1665363002/diff/120001/ppapi/proxy/ppapi_mess... > > File ppapi/proxy/ppapi_messages.h (right): > > > > > https://codereview.chromium.org/1665363002/diff/120001/ppapi/proxy/ppapi_mess... > > ppapi/proxy/ppapi_messages.h:493: int32_t /* renderer_pid */, > > no need to change right? > > Right, I'll remove it in next update. lgtm with that change Thank you!
https://codereview.chromium.org/1665363002/diff/120001/ipc/ipc_message_utils.h File ipc/ipc_message_utils.h (right): https://codereview.chromium.org/1665363002/diff/120001/ipc/ipc_message_utils.... ipc/ipc_message_utils.h:92: // NOT checked by 'IPC checker' Clang plugin. perhaps add a presubmit check to ensure that WriteParamUnchecked isn't accidentally used in more files?
On 2016/02/19 16:16:05, jam wrote: > https://codereview.chromium.org/1665363002/diff/120001/ipc/ipc_message_utils.h > File ipc/ipc_message_utils.h (right): > > https://codereview.chromium.org/1665363002/diff/120001/ipc/ipc_message_utils.... > ipc/ipc_message_utils.h:92: // NOT checked by 'IPC checker' Clang plugin. > perhaps add a presubmit check to ensure that WriteParamUnchecked isn't > accidentally used in more files? So the fear is that someone might just use it instead of complying with WriteParam() restrictions? We can make it's use a pain too, for example by renaming it to something long and weird.. Presubmit check would just limit its use to a set of files, but it won't actually ensure that its use is justified.
On 2016/02/19 19:48:06, Dmitry Skiba wrote: > On 2016/02/19 16:16:05, jam wrote: > > https://codereview.chromium.org/1665363002/diff/120001/ipc/ipc_message_utils.h > > File ipc/ipc_message_utils.h (right): > > > > > https://codereview.chromium.org/1665363002/diff/120001/ipc/ipc_message_utils.... > > ipc/ipc_message_utils.h:92: // NOT checked by 'IPC checker' Clang plugin. > > perhaps add a presubmit check to ensure that WriteParamUnchecked isn't > > accidentally used in more files? > > So the fear is that someone might just use it instead of complying with > WriteParam() restrictions? Correct > We can make it's use a pain too, for example by > renaming it to something long and weird.. Presubmit check would just limit its > use to a set of files, but it won't actually ensure that its use is justified. I meant use a presubmit check to limit it to say, src/ipc and ensure that at least you/tom/me are added as a reviewer.
Description was changed from ========== Clang plugin to check that unstable types are not used in IPC. We want IPC messages to be stable so that 32-bit and 64-bit processes can talk to each other. This plugin blacklists the following types that are known to be unstable: 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) Blacklisted types are not allowed in IPC::WriteParam() invocations or IPC::CheckedTuple<> specializations. BUG=581409 ========== to ========== Clang plugin to check that unstable types are not used in IPC. We want IPC messages to be stable so that 32-bit and 64-bit processes can talk to each other. This plugin blacklists the following types that are known to be unstable: 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) Blacklisted types are not allowed in IPC::WriteParam() invocations or IPC::CheckedTuple<> specializations. BUG=581409 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Clang guys, please review.
On 2016/02/22 at 17:57:20, dskiba wrote: > Clang guys, please review. I don't think we need a separate RecursiveASTVisitor still: can you please fold it into FindBadConstructsConsumer?
On 2016/02/22 18:13:52, dcheng wrote: > On 2016/02/22 at 17:57:20, dskiba wrote: > > Clang guys, please review. > > I don't think we need a separate RecursiveASTVisitor still: can you please fold > it into FindBadConstructsConsumer? What would be a benefit of that? CheckIPCVisitor is 1/2 of FindBadConstructsConsumer in size, doesn't need any of its utilities, and can be removed easily without a trace. Besides, conceptually CheckIPCVisitor doesn't find "bad constructs", it enforces correct usage of IPC machinery (hence chromium-ipc tag instead of chromium-style).
On 2016/02/22 at 18:32:38, dskiba wrote: > On 2016/02/22 18:13:52, dcheng wrote: > > On 2016/02/22 at 17:57:20, dskiba wrote: > > > Clang guys, please review. > > > > I don't think we need a separate RecursiveASTVisitor still: can you please fold > > it into FindBadConstructsConsumer? > > What would be a benefit of that? CheckIPCVisitor is 1/2 of FindBadConstructsConsumer in size, doesn't need any of its utilities, and can be removed easily without a trace. Besides, conceptually CheckIPCVisitor doesn't find "bad constructs", it enforces correct usage of IPC machinery (hence chromium-ipc tag instead of chromium-style). It doesn't seem scalable to keep adding one RecursiveASTVisitor per check. If every check was implemented like this, it seems like we'll perform n traversals of the AST to perform n checks.
On 2016/02/22 18:41:52, dcheng wrote: > On 2016/02/22 at 18:32:38, dskiba wrote: > > On 2016/02/22 18:13:52, dcheng wrote: > > > On 2016/02/22 at 17:57:20, dskiba wrote: > > > > Clang guys, please review. > > > > > > I don't think we need a separate RecursiveASTVisitor still: can you please > fold > > > it into FindBadConstructsConsumer? > > > > What would be a benefit of that? CheckIPCVisitor is 1/2 of > FindBadConstructsConsumer in size, doesn't need any of its utilities, and can be > removed easily without a trace. Besides, conceptually CheckIPCVisitor doesn't > find "bad constructs", it enforces correct usage of IPC machinery (hence > chromium-ipc tag instead of chromium-style). > > It doesn't seem scalable to keep adding one RecursiveASTVisitor per check. If > every check was implemented like this, it seems like we'll perform n traversals > of the AST to perform n checks. So you worry about the extra traversal added, and think that without that performance will be (measurably) better? I don't think CheckIPCVisitor sets a worrying trend, it's an exception, and all future checks should be added to FindBadConstructsConsumer as usual.
On 2016/02/22 at 18:57:25, dskiba wrote: > On 2016/02/22 18:41:52, dcheng wrote: > > On 2016/02/22 at 18:32:38, dskiba wrote: > > > On 2016/02/22 18:13:52, dcheng wrote: > > > > On 2016/02/22 at 17:57:20, dskiba wrote: > > > > > Clang guys, please review. > > > > > > > > I don't think we need a separate RecursiveASTVisitor still: can you please > > fold > > > > it into FindBadConstructsConsumer? > > > > > > What would be a benefit of that? CheckIPCVisitor is 1/2 of > > FindBadConstructsConsumer in size, doesn't need any of its utilities, and can be > > removed easily without a trace. Besides, conceptually CheckIPCVisitor doesn't > > find "bad constructs", it enforces correct usage of IPC machinery (hence > > chromium-ipc tag instead of chromium-style). > > > > It doesn't seem scalable to keep adding one RecursiveASTVisitor per check. If > > every check was implemented like this, it seems like we'll perform n traversals > > of the AST to perform n checks. > > So you worry about the extra traversal added, and think that without that performance will be (measurably) better? > > I don't think CheckIPCVisitor sets a worrying trend, it's an exception, and all future checks should be added to FindBadConstructsConsumer as usual. What makes this code special enough to have its own RAV? Following that line of reasoning, why should future checks not be separate RAVs as well?
On 2016/02/22 18:59:04, dcheng wrote: > On 2016/02/22 at 18:57:25, dskiba wrote: > > On 2016/02/22 18:41:52, dcheng wrote: > > > On 2016/02/22 at 18:32:38, dskiba wrote: > > > > On 2016/02/22 18:13:52, dcheng wrote: > > > > > On 2016/02/22 at 17:57:20, dskiba wrote: > > > > > > Clang guys, please review. > > > > > > > > > > I don't think we need a separate RecursiveASTVisitor still: can you > please > > > fold > > > > > it into FindBadConstructsConsumer? > > > > > > > > What would be a benefit of that? CheckIPCVisitor is 1/2 of > > > FindBadConstructsConsumer in size, doesn't need any of its utilities, and > can be > > > removed easily without a trace. Besides, conceptually CheckIPCVisitor > doesn't > > > find "bad constructs", it enforces correct usage of IPC machinery (hence > > > chromium-ipc tag instead of chromium-style). > > > > > > It doesn't seem scalable to keep adding one RecursiveASTVisitor per check. > If > > > every check was implemented like this, it seems like we'll perform n > traversals > > > of the AST to perform n checks. > > > > So you worry about the extra traversal added, and think that without that > performance will be (measurably) better? > > > > I don't think CheckIPCVisitor sets a worrying trend, it's an exception, and > all future checks should be added to FindBadConstructsConsumer as usual. > > What makes this code special enough to have its own RAV? Following that line of > reasoning, why should future checks not be separate RAVs as well? Because it implements several (four) checks enforcing one feature (blacklisted IPC types). Those checks are quite hefty, and logically separate from the types of checks FindBadConstructsConsumer is doing already. I'm afraid that merging those two classes will create a mess. However, if extra traversal is a concern, I can think of a solution - CheckIPCVisitor will remain separate, but it won't be RAV, and but will be driven by FindBadConstructsConsumer instead.
On 2016/02/22 at 19:08:42, dskiba wrote: > On 2016/02/22 18:59:04, dcheng wrote: > > On 2016/02/22 at 18:57:25, dskiba wrote: > > > On 2016/02/22 18:41:52, dcheng wrote: > > > > On 2016/02/22 at 18:32:38, dskiba wrote: > > > > > On 2016/02/22 18:13:52, dcheng wrote: > > > > > > On 2016/02/22 at 17:57:20, dskiba wrote: > > > > > > > Clang guys, please review. > > > > > > > > > > > > I don't think we need a separate RecursiveASTVisitor still: can you > > please > > > > fold > > > > > > it into FindBadConstructsConsumer? > > > > > > > > > > What would be a benefit of that? CheckIPCVisitor is 1/2 of > > > > FindBadConstructsConsumer in size, doesn't need any of its utilities, and > > can be > > > > removed easily without a trace. Besides, conceptually CheckIPCVisitor > > doesn't > > > > find "bad constructs", it enforces correct usage of IPC machinery (hence > > > > chromium-ipc tag instead of chromium-style). > > > > > > > > It doesn't seem scalable to keep adding one RecursiveASTVisitor per check. > > If > > > > every check was implemented like this, it seems like we'll perform n > > traversals > > > > of the AST to perform n checks. > > > > > > So you worry about the extra traversal added, and think that without that > > performance will be (measurably) better? > > > > > > I don't think CheckIPCVisitor sets a worrying trend, it's an exception, and > > all future checks should be added to FindBadConstructsConsumer as usual. > > > > What makes this code special enough to have its own RAV? Following that line of > > reasoning, why should future checks not be separate RAVs as well? > > Because it implements several (four) checks enforcing one feature (blacklisted IPC types). Those checks are quite hefty, and logically separate from the types of checks FindBadConstructsConsumer is doing already. I'm afraid that merging those two classes will create a mess. > > However, if extra traversal is a concern, I can think of a solution - CheckIPCVisitor will remain separate, but it won't be RAV, and but will be driven by FindBadConstructsConsumer instead. Sounds good, thanks.
On 2016/02/22 19:19:55, dcheng wrote: > On 2016/02/22 at 19:08:42, dskiba wrote: > > On 2016/02/22 18:59:04, dcheng wrote: > > > On 2016/02/22 at 18:57:25, dskiba wrote: > > > > On 2016/02/22 18:41:52, dcheng wrote: > > > > > On 2016/02/22 at 18:32:38, dskiba wrote: > > > > > > On 2016/02/22 18:13:52, dcheng wrote: > > > > > > > On 2016/02/22 at 17:57:20, dskiba wrote: > > > > > > > > Clang guys, please review. > > > > > > > > > > > > > > I don't think we need a separate RecursiveASTVisitor still: can you > > > please > > > > > fold > > > > > > > it into FindBadConstructsConsumer? > > > > > > > > > > > > What would be a benefit of that? CheckIPCVisitor is 1/2 of > > > > > FindBadConstructsConsumer in size, doesn't need any of its utilities, > and > > > can be > > > > > removed easily without a trace. Besides, conceptually CheckIPCVisitor > > > doesn't > > > > > find "bad constructs", it enforces correct usage of IPC machinery (hence > > > > > chromium-ipc tag instead of chromium-style). > > > > > > > > > > It doesn't seem scalable to keep adding one RecursiveASTVisitor per > check. > > > If > > > > > every check was implemented like this, it seems like we'll perform n > > > traversals > > > > > of the AST to perform n checks. > > > > > > > > So you worry about the extra traversal added, and think that without that > > > performance will be (measurably) better? > > > > > > > > I don't think CheckIPCVisitor sets a worrying trend, it's an exception, > and > > > all future checks should be added to FindBadConstructsConsumer as usual. > > > > > > What makes this code special enough to have its own RAV? Following that line > of > > > reasoning, why should future checks not be separate RAVs as well? > > > > Because it implements several (four) checks enforcing one feature (blacklisted > IPC types). Those checks are quite hefty, and logically separate from the types > of checks FindBadConstructsConsumer is doing already. I'm afraid that merging > those two classes will create a mess. > > > > However, if extra traversal is a concern, I can think of a solution - > CheckIPCVisitor will remain separate, but it won't be RAV, and but will be > driven by FindBadConstructsConsumer instead. > > Sounds good, thanks. Guys, check-ipc switch is turned on only for Android builds. Do we have any bots that build for Android using Clang plugins? android_clang_dbg_recipe seems to be the one, but it didn't fail when I triggered it for patchset 10 (which was before GPU IPC fix). So, should this land before bots can use it, or do we have Android find-bad-constructs bot at all?
On 2016/02/23 at 18:38:03, dskiba wrote: > On 2016/02/22 19:19:55, dcheng wrote: > > On 2016/02/22 at 19:08:42, dskiba wrote: > > > On 2016/02/22 18:59:04, dcheng wrote: > > > > On 2016/02/22 at 18:57:25, dskiba wrote: > > > > > On 2016/02/22 18:41:52, dcheng wrote: > > > > > > On 2016/02/22 at 18:32:38, dskiba wrote: > > > > > > > On 2016/02/22 18:13:52, dcheng wrote: > > > > > > > > On 2016/02/22 at 17:57:20, dskiba wrote: > > > > > > > > > Clang guys, please review. > > > > > > > > > > > > > > > > I don't think we need a separate RecursiveASTVisitor still: can you > > > > please > > > > > > fold > > > > > > > > it into FindBadConstructsConsumer? > > > > > > > > > > > > > > What would be a benefit of that? CheckIPCVisitor is 1/2 of > > > > > > FindBadConstructsConsumer in size, doesn't need any of its utilities, > > and > > > > can be > > > > > > removed easily without a trace. Besides, conceptually CheckIPCVisitor > > > > doesn't > > > > > > find "bad constructs", it enforces correct usage of IPC machinery (hence > > > > > > chromium-ipc tag instead of chromium-style). > > > > > > > > > > > > It doesn't seem scalable to keep adding one RecursiveASTVisitor per > > check. > > > > If > > > > > > every check was implemented like this, it seems like we'll perform n > > > > traversals > > > > > > of the AST to perform n checks. > > > > > > > > > > So you worry about the extra traversal added, and think that without that > > > > performance will be (measurably) better? > > > > > > > > > > I don't think CheckIPCVisitor sets a worrying trend, it's an exception, > > and > > > > all future checks should be added to FindBadConstructsConsumer as usual. > > > > > > > > What makes this code special enough to have its own RAV? Following that line > > of > > > > reasoning, why should future checks not be separate RAVs as well? > > > > > > Because it implements several (four) checks enforcing one feature (blacklisted > > IPC types). Those checks are quite hefty, and logically separate from the types > > of checks FindBadConstructsConsumer is doing already. I'm afraid that merging > > those two classes will create a mess. > > > > > > However, if extra traversal is a concern, I can think of a solution - > > CheckIPCVisitor will remain separate, but it won't be RAV, and but will be > > driven by FindBadConstructsConsumer instead. > > > > Sounds good, thanks. > > Guys, check-ipc switch is turned on only for Android builds. Do we have any bots that build for Android using Clang plugins? > > android_clang_dbg_recipe seems to be the one, but it didn't fail when I triggered it for patchset 10 (which was before GPU IPC fix). So, should this land before bots can use it, or do we have Android find-bad-constructs bot at all? The bots won't use your custom plugin. AFAIK, there's no officially sanctioned way of testing the custom plugin on the bots.
On 2016/02/23 at 18:42:06, dcheng wrote: > On 2016/02/23 at 18:38:03, dskiba wrote: > > On 2016/02/22 19:19:55, dcheng wrote: > > > On 2016/02/22 at 19:08:42, dskiba wrote: > > > > On 2016/02/22 18:59:04, dcheng wrote: > > > > > On 2016/02/22 at 18:57:25, dskiba wrote: > > > > > > On 2016/02/22 18:41:52, dcheng wrote: > > > > > > > On 2016/02/22 at 18:32:38, dskiba wrote: > > > > > > > > On 2016/02/22 18:13:52, dcheng wrote: > > > > > > > > > On 2016/02/22 at 17:57:20, dskiba wrote: > > > > > > > > > > Clang guys, please review. > > > > > > > > > > > > > > > > > > I don't think we need a separate RecursiveASTVisitor still: can you > > > > > please > > > > > > > fold > > > > > > > > > it into FindBadConstructsConsumer? > > > > > > > > > > > > > > > > What would be a benefit of that? CheckIPCVisitor is 1/2 of > > > > > > > FindBadConstructsConsumer in size, doesn't need any of its utilities, > > > and > > > > > can be > > > > > > > removed easily without a trace. Besides, conceptually CheckIPCVisitor > > > > > doesn't > > > > > > > find "bad constructs", it enforces correct usage of IPC machinery (hence > > > > > > > chromium-ipc tag instead of chromium-style). > > > > > > > > > > > > > > It doesn't seem scalable to keep adding one RecursiveASTVisitor per > > > check. > > > > > If > > > > > > > every check was implemented like this, it seems like we'll perform n > > > > > traversals > > > > > > > of the AST to perform n checks. > > > > > > > > > > > > So you worry about the extra traversal added, and think that without that > > > > > performance will be (measurably) better? > > > > > > > > > > > > I don't think CheckIPCVisitor sets a worrying trend, it's an exception, > > > and > > > > > all future checks should be added to FindBadConstructsConsumer as usual. > > > > > > > > > > What makes this code special enough to have its own RAV? Following that line > > > of > > > > > reasoning, why should future checks not be separate RAVs as well? > > > > > > > > Because it implements several (four) checks enforcing one feature (blacklisted > > > IPC types). Those checks are quite hefty, and logically separate from the types > > > of checks FindBadConstructsConsumer is doing already. I'm afraid that merging > > > those two classes will create a mess. > > > > > > > > However, if extra traversal is a concern, I can think of a solution - > > > CheckIPCVisitor will remain separate, but it won't be RAV, and but will be > > > driven by FindBadConstructsConsumer instead. > > > > > > Sounds good, thanks. > > > > Guys, check-ipc switch is turned on only for Android builds. Do we have any bots that build for Android using Clang plugins? > > > > android_clang_dbg_recipe seems to be the one, but it didn't fail when I triggered it for patchset 10 (which was before GPU IPC fix). So, should this land before bots can use it, or do we have Android find-bad-constructs bot at all? > > The bots won't use your custom plugin. AFAIK, there's no officially sanctioned way of testing the custom plugin on the bots. To be more precise, what's happening is the bots are using the prebuilt clang binaries (including the plugin). Even if the bots were using your plugin, they generally use goma by default, and goma will not have your custom plugin.
> The bots won't use your custom plugin. AFAIK, there's no officially sanctioned > way of testing the custom plugin on the bots. OK, "my plugin" was badly phrased - "check-ipc option to find-bad-constructs plugin" is more accurate. See build/common.gypi and build/config/clang/BUILD.gn where I enable "check-ipc" only if it's Android build. We do have bots that run find-bad-constructs plugin, right? But non of the bots failed for patchset 10, which had an unfixed issue (fixed 11). So I'm wondering why.
> To be more precise, what's happening is the bots are using the prebuilt clang > binaries (including the plugin). Even if the bots were using your plugin, they > generally use goma by default, and goma will not have your custom plugin. Oh I see. So I first need to land this, then wait for new GOMA release, and only then it'll start to function? Do you know where I can look up bots configs, to be sure that CQ has at least one Android builder which builds with find-bad-constructs?
On 2016/02/23 at 18:48:51, dskiba wrote: > > The bots won't use your custom plugin. AFAIK, there's no officially sanctioned > > way of testing the custom plugin on the bots. > > OK, "my plugin" was badly phrased - "check-ipc option to find-bad-constructs plugin" is more accurate. See build/common.gypi and build/config/clang/BUILD.gn where I enable "check-ipc" only if it's Android build. We do have bots that run find-bad-constructs plugin, right? > Right, if you look at the android clang build logs, you'll see this: [18839/22578] CXX obj/net/http/net_unittests.http_network_layer_unittest.o Unknown clang plugin argument: check-ipc > But non of the bots failed for patchset 10, which had an unfixed issue (fixed 11). So I'm wondering why. The bots are using prebuilt clang packages, which includes the compiler *and* the clang plugins. The bot won't automatically rebuild clang, so it won't have the changes that you made to the plugin. Even if it did know how to do that, the bots use goma, and the goma builders won't have your updated plugin either. If you want to test plugin changes, it's generally easiest to do it locally. To actually get the change to stick in trunk, you need to do something like this: 1. Land your clang plugin changes. 2. Land fixes to trunk for any new warnings/errors. 3. Wait for the prebuilt clang packages to be rolled and include your plugin changes. 4. Land a patch that enables the new check in BUILD.gn/common.gypi, fixing any new errors that have appeared. 5. Clean up the now unneeded plugin flag.
On 2016/02/23 18:53:01, dcheng wrote: > On 2016/02/23 at 18:48:51, dskiba wrote: > > > The bots won't use your custom plugin. AFAIK, there's no officially > sanctioned > > > way of testing the custom plugin on the bots. > > > > OK, "my plugin" was badly phrased - "check-ipc option to find-bad-constructs > plugin" is more accurate. See build/common.gypi and build/config/clang/BUILD.gn > where I enable "check-ipc" only if it's Android build. We do have bots that run > find-bad-constructs plugin, right? > > > > Right, if you look at the android clang build logs, you'll see this: > [18839/22578] CXX obj/net/http/net_unittests.http_network_layer_unittest.o > Unknown clang plugin argument: check-ipc > > > > But non of the bots failed for patchset 10, which had an unfixed issue (fixed > 11). So I'm wondering why. > > The bots are using prebuilt clang packages, which includes the compiler *and* > the clang plugins. The bot won't automatically rebuild clang, so it won't have > the changes that you made to the plugin. > Even if it did know how to do that, the bots use goma, and the goma builders > won't have your updated plugin either. If you want to test plugin changes, it's > generally easiest to do it locally. > > To actually get the change to stick in trunk, you need to do something like > this: > 1. Land your clang plugin changes. > 2. Land fixes to trunk for any new warnings/errors. > 3. Wait for the prebuilt clang packages to be rolled and include your plugin > changes. > 4. Land a patch that enables the new check in BUILD.gn/common.gypi, fixing any > new errors that have appeared. > 5. Clean up the now unneeded plugin flag. Thanks! This answers all my questions. I'll start with reverting changes to BUILD.gn/common.gypi. Did you have a chance to review how I removed RAV from CheckIPCVisitor?
https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.cpp (right): https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:33: ""; Add some content to this string? https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:47: static llvm::StringSet<> blacklist({ No static initializers. Just make this an instance var? https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:84: if (auto tdef = dyn_cast<TypedefType>(type)) { Please qualify auto with * or & as appropriate (it should be auto* in most places. Also, why dyn_cast here and getAs<TypedefType> in other places? https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:95: type->getLocallyUnqualifiedSingleStepDesugaredType(); The documentation for this says to prefer getSingleStepDesugaredType(): does that work here? https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:130: for (unsigned i = 0; i != spec->getNumArgs(); ++i) { I think you can just do for (TemplateArgument* arg : spec) https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:139: auto spec = dyn_cast<ClassTemplateSpecializationDecl>(record->getDecl()); Fold into 140 for consistency. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:164: unsigned error, unsigned typedef_note) { What's the point of typedef_note: aren't we always passing the same thing in? https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:304: CheckType(*context_, arg_type, &details); Why not just pass details in to begin with? https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.h (right): https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:5: // This plugin ensures that 32/64-bit unstable types are not used in IPC. s/plugin/check/ https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:7: // Type (or typedef) is unstable if it changes size between 32/ 64-bit Nit: s/Type/A type/ https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:22: // is IPC::ParamTraits<> specialization s/is IPC/is an IPC/ https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:25: // template arguments are checked by #2 Hmm... is it possible to fix this shortcoming? It's kind of unfortunate, the whole point of WriteParam is to be a type-deducing helper. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:48: bool shouldVisitTemplateInstantiations() const { return true; } NameLikeThis or name_like_this please. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:56: /* ValidateXXX functions return false if validation failed and diagnostic Nit: Use C++ style // comments for consistency. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/te... File tools/clang/plugins/tests/ipc.flags (right): https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.flags:1: -ferror-limit=50 -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -ferror-limit=0? 50 seems kind of arbitrary.
TBD is getting rid of static in IsBlacklistedTypedef(). https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.cpp (right): https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:33: ""; On 2016/02/23 19:46:46, dcheng wrote: > Add some content to this string? Done. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:47: static llvm::StringSet<> blacklist({ On 2016/02/23 19:46:46, dcheng wrote: > No static initializers. Just make this an instance var? TBD https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:84: if (auto tdef = dyn_cast<TypedefType>(type)) { On 2016/02/23 19:46:46, dcheng wrote: > Please qualify auto with * or & as appropriate (it should be auto* in most > places. > > Also, why dyn_cast here and getAs<TypedefType> in other places? Done. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:95: type->getLocallyUnqualifiedSingleStepDesugaredType(); On 2016/02/23 19:46:46, dcheng wrote: > The documentation for this says to prefer getSingleStepDesugaredType(): does > that work here? Previously I was calling that function / stripping away quals, but getLocallyUnqualifiedSingleStepDesugaredType() achieves that in a single step (actually, getSingleStepDesugaredType() is calling getLocallyUnqualifiedSingleStepDesugaredType() and then applying quals back). https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:130: for (unsigned i = 0; i != spec->getNumArgs(); ++i) { On 2016/02/23 19:46:46, dcheng wrote: > I think you can just do for (TemplateArgument* arg : spec) Done. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:139: auto spec = dyn_cast<ClassTemplateSpecializationDecl>(record->getDecl()); On 2016/02/23 19:46:46, dcheng wrote: > Fold into 140 for consistency. Done. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:164: unsigned error, unsigned typedef_note) { On 2016/02/23 19:46:46, dcheng wrote: > What's the point of typedef_note: aren't we always passing the same thing in? Yes, but it's not known here, since ReportCheckError() is not inside CheckIPCVisitor. However, it seems that I'll move everything into the class, so this function will look different. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:304: CheckType(*context_, arg_type, &details); On 2016/02/23 19:46:46, dcheng wrote: > Why not just pass details in to begin with? It's just an optimization. I expect CheckType() to fail most of the time, and collecting details involves pushing_back to vector, which I want to avoid. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.h (right): https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:5: // This plugin ensures that 32/64-bit unstable types are not used in IPC. On 2016/02/23 19:46:47, dcheng wrote: > s/plugin/check/ Done. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:7: // Type (or typedef) is unstable if it changes size between 32/ 64-bit On 2016/02/23 19:46:47, dcheng wrote: > Nit: s/Type/A type/ Done. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:22: // is IPC::ParamTraits<> specialization On 2016/02/23 19:46:46, dcheng wrote: > s/is IPC/is an IPC/ Done. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:25: // template arguments are checked by #2 On 2016/02/23 19:46:47, dcheng wrote: > Hmm... is it possible to fix this shortcoming? It's kind of unfortunate, the > whole point of WriteParam is to be a type-deducing helper. Yes, but that only for templates. Everywhere else WriteParam() is fully permitted and still deduces types as usual. The issue with templates is that we're blacklisting typedefs, but that information exists only at the "top level". I.e. template <class T> void DoWrite(const T& value) { WriteParam(value); } DoWrite<size_t>() DoWrite<unsigned int>() Both DoWrite calls will use the same unsigned int specialization (on 32-bit Android), but we must fail on WriteParam() inside the first one. The proper way is to build some system to infer types back to their typedefs outside of templates, but that's hard and probably not doable in a plugin where we can only observe final AST. So I went different path - I prohibit usage of WriteParam() in templates, with the exception of ParamTraits<> specializations. And inside ParamTraits<> I simply don't check WriteParam() arguments at all. That mostly works, because usually for a given IPC message WriteParam() ends up being called on everything that was listed, i.e. for IPC_MESSAGE((bool, MyClass, std::vector<char>)) we expect WriteParam() calls on bool, MyClass, std::vector<> and char. IPC_MESSAGE uses IPC::CheckedTuple<> which recursively checks arguments, so IPC_MESSAGE((std::vector<std::pair<bool, size_t>>)) fails. However, there is a chance that ParamTraits<> might call WriteParam() on something that was not checked by IPC::CheckedTuple<>. For example on size_type: ParamTraits<std::vector<P>>::Write(vec) { WriteParam(vec.size()); } To protect from that I require WriteParam() calls to explicitly reference template argument. Or to explicitly cast to something that doesn't depend on template argument, in which case it's checked by a standard WriteParam() check (ran on template itself). As you can see from ipc_message_utils.h most explicit WriteParam specializations are straightforward. And for corner cases we have WriteParamUnchecked() (see SmallMap specialization). https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:48: bool shouldVisitTemplateInstantiations() const { return true; } On 2016/02/23 19:46:47, dcheng wrote: > NameLikeThis or name_like_this please. Done. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:56: /* ValidateXXX functions return false if validation failed and diagnostic On 2016/02/23 19:46:47, dcheng wrote: > Nit: Use C++ style // comments for consistency. Done. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/te... File tools/clang/plugins/tests/ipc.flags (right): https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.flags:1: -ferror-limit=50 -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc On 2016/02/23 19:46:47, dcheng wrote: > -ferror-limit=0? > > 50 seems kind of arbitrary. Done.
https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.cpp (right): https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:304: CheckType(*context_, arg_type, &details); On 2016/02/24 at 14:39:04, Dmitry Skiba wrote: > On 2016/02/23 19:46:46, dcheng wrote: > > Why not just pass details in to begin with? > > It's just an optimization. I expect CheckType() to fail most of the time, and collecting details involves pushing_back to vector, which I want to avoid. I'd just use llvm::SmallVector<> and always pass in a CheckDetails. CheckDetails is stack allocated in this code, and SmallVector has room for a couple inline elements, so we can avoid spilling to the heap most of the time. https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.h (right): https://codereview.chromium.org/1665363002/diff/240001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:25: // template arguments are checked by #2 On 2016/02/24 at 14:39:04, Dmitry Skiba wrote: > On 2016/02/23 19:46:47, dcheng wrote: > > Hmm... is it possible to fix this shortcoming? It's kind of unfortunate, the > > whole point of WriteParam is to be a type-deducing helper. > > Yes, but that only for templates. Everywhere else WriteParam() is fully permitted and still deduces types as usual. > > The issue with templates is that we're blacklisting typedefs, but that information exists only at the "top level". I.e. > > template <class T> > void DoWrite(const T& value) { > WriteParam(value); > } > > DoWrite<size_t>() > DoWrite<unsigned int>() > > Both DoWrite calls will use the same unsigned int specialization (on 32-bit Android), but we must fail on WriteParam() inside the first one. The proper way is to build some system to infer types back to their typedefs outside of templates, but that's hard and probably not doable in a plugin where we can only observe final AST. > > So I went different path - I prohibit usage of WriteParam() in templates, with the exception of ParamTraits<> specializations. And inside ParamTraits<> I simply don't check WriteParam() arguments at all. That mostly works, because usually for a given IPC message WriteParam() ends up being called on everything that was listed, i.e. for > > IPC_MESSAGE((bool, MyClass, std::vector<char>)) > > we expect WriteParam() calls on bool, MyClass, std::vector<> and char. IPC_MESSAGE uses IPC::CheckedTuple<> which recursively checks arguments, so IPC_MESSAGE((std::vector<std::pair<bool, size_t>>)) fails. > > However, there is a chance that ParamTraits<> might call WriteParam() on something that was not checked by IPC::CheckedTuple<>. For example on size_type: > > ParamTraits<std::vector<P>>::Write(vec) { > WriteParam(vec.size()); > } > So assume I have a clang::CallExpr representing the WriteParam() call above. Does that mean that write_param_call->getArg(0)->getType() returns a QualType like "unsigned int" instead of "size_t"? Alternatively, can we wait until the template is instantiated and we have concrete types and try to emit the warning then? > To protect from that I require WriteParam() calls to explicitly reference template argument. Or to explicitly cast to something that doesn't depend on template argument, in which case it's checked by a standard WriteParam() check (ran on template itself). > > As you can see from ipc_message_utils.h most explicit WriteParam specializations are straightforward. And for corner cases we have WriteParamUnchecked() (see SmallMap specialization). It's simple, but still an anti-pattern. And it has the very real possibility that the explicitly-specified template arg might differ from the actual type of the argument. If there's any way to avoid this, we should try to find it.
On 2016/02/24 22:57:42, dcheng wrote: ... > > It's just an optimization. I expect CheckType() to fail most of the time, and > collecting details involves pushing_back to vector, which I want to avoid. > > I'd just use llvm::SmallVector<> and always pass in a CheckDetails. CheckDetails > is stack allocated in this code, and SmallVector has room for a couple inline > elements, so we can avoid spilling to the heap most of the time. Yeah, that would work too. I don't have a preference here - do you want me to change to SmallVector? ... > So assume I have a clang::CallExpr representing the WriteParam() call above. > Does that mean that write_param_call->getArg(0)->getType() returns a QualType > like "unsigned int" instead of "size_t"? > Alternatively, can we wait until the template is instantiated and we have > concrete types and try to emit the warning then? That's right - if you have a template specialized with size_t, which then calls WriteParam() on template argument, then inside instantiation you'll see that WriteParam() is called on unsigned int (or whatever underlying type is). It's only at the point of specialization, where you have template arguments "as written", you have size_t. This is how CheckedTuple<> works - arguments from IPC_MESSAGE macros are just pasted between the brackets, so CheckedTuple<> specializations always see types as written in macros. ... > It's simple, but still an anti-pattern. And it has the very real possibility > that the explicitly-specified template arg might differ from the actual type of > the argument. If there's any way to avoid this, we should try to find it. Yeah, it's not ideal, but it's the best thing I can came up with after spending days on the problem. It's certainly better than not doing any checks at all - since at least we make template writers think about what is being serialized. John proposed to create a presubmit hook to make sure WriteParamUnchecked() is correctly used - that will help too. But it's completely possible that I overlooked something - if you have any ideas, I would love to hear them! You point about T in WriteParam<T>() being different from actual argument type is valid. Indeed, for WriteParam<T>(arg) compiler will try its best to convert arg to T. Do you have thoughts on how to disable those conversions?
On 2016/02/25 at 17:37:07, dskiba wrote: > On 2016/02/24 22:57:42, dcheng wrote: > ... > > > It's just an optimization. I expect CheckType() to fail most of the time, and > > collecting details involves pushing_back to vector, which I want to avoid. > > > > I'd just use llvm::SmallVector<> and always pass in a CheckDetails. CheckDetails > > is stack allocated in this code, and SmallVector has room for a couple inline > > elements, so we can avoid spilling to the heap most of the time. > > Yeah, that would work too. I don't have a preference here - do you want me to change to SmallVector? > Yes, let's do this. > ... > > So assume I have a clang::CallExpr representing the WriteParam() call above. > > Does that mean that write_param_call->getArg(0)->getType() returns a QualType > > like "unsigned int" instead of "size_t"? > > Alternatively, can we wait until the template is instantiated and we have > > concrete types and try to emit the warning then? > > That's right - if you have a template specialized with size_t, which then calls WriteParam() on template argument, then inside instantiation you'll see that WriteParam() is called on unsigned int (or whatever underlying type is). It's only at the point of specialization, where you have template arguments "as written", you have size_t. This is how CheckedTuple<> works - arguments from IPC_MESSAGE macros are just pasted between the brackets, so CheckedTuple<> specializations always see types as written in macros. > > > ... > > It's simple, but still an anti-pattern. And it has the very real possibility > > that the explicitly-specified template arg might differ from the actual type of > > the argument. If there's any way to avoid this, we should try to find it. > > Yeah, it's not ideal, but it's the best thing I can came up with after spending days on the problem. It's certainly better than not doing any checks at all - since at least we make template writers think about what is being serialized. John proposed to create a presubmit hook to make sure WriteParamUnchecked() is correctly used - that will help too. > > But it's completely possible that I overlooked something - if you have any ideas, I would love to hear them! > > You point about T in WriteParam<T>() being different from actual argument type is valid. Indeed, for WriteParam<T>(arg) compiler will try its best to convert arg to T. Do you have thoughts on how to disable those conversions? I need to think about this some more, sorry. This is tricky. I'll try asking some people who hack on clang if they have any ideas as well.
On 2016/02/26 18:40:50, dcheng wrote: ... > Yes, let's do this. I was really busy last week; I'll do it today / tomorrow. ... > I need to think about this some more, sorry. This is tricky. I'll try asking > some people who hack on clang if they have any ideas as well. Thanks for that, I appreciate it. If you happen to start any public discussion, please drop a link here. Maybe we should split this commit in two parts? Even without code to check WriteParam() usage in templates the plugin is still useful. And then once we agree on how to handle templates, I'll push the second part. John, what do you think?
On 2016/02/29 at 16:42:05, dskiba wrote: > On 2016/02/26 18:40:50, dcheng wrote: > ... > > Yes, let's do this. > > I was really busy last week; I'll do it today / tomorrow. > > ... > > I need to think about this some more, sorry. This is tricky. I'll try asking > > some people who hack on clang if they have any ideas as well. > > Thanks for that, I appreciate it. If you happen to start any public discussion, please drop a link here. > > Maybe we should split this commit in two parts? Even without code to check WriteParam() usage in templates the plugin is still useful. And then once we agree on how to handle templates, I'll push the second part. > > John, what do you think? This seems reasonable to me if jam@ is OK with it.
On 2016/02/29 17:50:01, dcheng wrote: > On 2016/02/29 at 16:42:05, dskiba wrote: > > On 2016/02/26 18:40:50, dcheng wrote: > > ... > > > Yes, let's do this. > > > > I was really busy last week; I'll do it today / tomorrow. > > > > ... > > > I need to think about this some more, sorry. This is tricky. I'll try asking > > > some people who hack on clang if they have any ideas as well. > > > > Thanks for that, I appreciate it. If you happen to start any public > discussion, please drop a link here. > > > > Maybe we should split this commit in two parts? Even without code to check > WriteParam() usage in templates the plugin is still useful. And then once we > agree on how to handle templates, I'll push the second part. > > > > John, what do you think? > > This seems reasonable to me if jam@ is OK with it. yep definitely, landing it in stages gives us a clear benefit at each stage. lgtm
Daniel, please review. I think I addressed everything.
https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.cpp (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:119: if (auto* cast_expr = dyn_cast<ImplicitCastExpr>(arg_type_expr)) { Nit: } else if {auto* cast_expr = dyn_cast<ImplicitCastExr>(...)) { arg_type_expr = cast_expr->getSubExpr(); } else { arg_type = arg_type_expr->getType(); } https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:222: if (details->entry_type == QualType()) { Does details->entry_type.isNull() work here? https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.h (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:48: void BeginDecl(clang::Decl*); We should probably either consistently include or exclude "obvious" parameter names (for example, compare to lines 58-64) https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:64: bool ValidateCheckedTuple( Nit: No newlines on 59, 61, 63, 71, 81, 83 to help group the methods a bit more visually. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... tools/clang/plugins/FindBadConstructsConsumer.cpp:150: return RecursiveASTVisitor::shouldVisitTemplateInstantiations() || I don't think we need to call the 'base' method here, it's not very interesting. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... tools/clang/plugins/FindBadConstructsConsumer.cpp:158: return result; Seems like this should all just be in VisitDecl() for consistency with RAV's naming schemes (and we won't need to manually dispatch to the 'base' implementation either). https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... File tools/clang/plugins/FindBadConstructsConsumer.h (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... tools/clang/plugins/FindBadConstructsConsumer.h:49: void Visit(clang::ASTContext& context); Nit: Traverse() for consistency with RecursiveASTVisitor's naming scheme. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... File tools/clang/plugins/tests/ipc.cpp (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.cpp:48: template <class... T> No indent in namespace (similar comment above) https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.cpp:64: #if 1 Remove the #if 1's. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.cpp:126: // NOT CHECKED: WriteParam() can only be used in ParamTraits templates I don't think I understand how this comment relates to 127-129: 128 is still an error with the ipc checker. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.cpp:134: // NOT CHECKED: WriteParam() can only be used in ParamTraits templates Same here. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.cpp:143: // NOT CHECKED: WriteParam() can only be used in ParamTraits templates And here. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.cpp:234: IPC::WriteParam(nullptr, p + 1); // ERROR Slightly unexpected. I guess this basically shouldn't come up at all though. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.cpp:328: IPC::WriteParam(nullptr, p.get<uint64_t>()); // ERROR I guess this is just for the template case, so maybe it's not too bad.
https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.cpp (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:119: if (auto* cast_expr = dyn_cast<ImplicitCastExpr>(arg_type_expr)) { On 2016/03/07 23:44:39, dcheng wrote: > Nit: > > } else if {auto* cast_expr = dyn_cast<ImplicitCastExr>(...)) { > arg_type_expr = cast_expr->getSubExpr(); > } else { > arg_type = arg_type_expr->getType(); > } Hmm, but in original code arg_type assignment happens in both branches. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:222: if (details->entry_type == QualType()) { On 2016/03/07 23:44:39, dcheng wrote: > Does details->entry_type.isNull() work here? Done. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.h (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:48: void BeginDecl(clang::Decl*); On 2016/03/07 23:44:39, dcheng wrote: > We should probably either consistently include or exclude "obvious" parameter > names (for example, compare to lines 58-64) Done. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.h:64: bool ValidateCheckedTuple( On 2016/03/07 23:44:39, dcheng wrote: > Nit: No newlines on 59, 61, 63, 71, 81, 83 to help group the methods a bit more > visually. Done. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... File tools/clang/plugins/FindBadConstructsConsumer.cpp (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... tools/clang/plugins/FindBadConstructsConsumer.cpp:150: return RecursiveASTVisitor::shouldVisitTemplateInstantiations() || On 2016/03/07 23:44:39, dcheng wrote: > I don't think we need to call the 'base' method here, it's not very interesting. Yes, but it provides the value I "blend" with. I don't want to solely decide what this method should return, I just want to make sure it respects what ipc_visitor wants. Having base call also makes it easy to see that method should be removed once ipc_message is removed, since in that case method collapses to a single base call. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... tools/clang/plugins/FindBadConstructsConsumer.cpp:158: return result; On 2016/03/07 23:44:39, dcheng wrote: > Seems like this should all just be in VisitDecl() for consistency with RAV's > naming schemes (and we won't need to manually dispatch to the 'base' > implementation either). VisitDecl() is called on leafs, so I won't be able to create stack of current decls, see GetParentDecl() and its uses in CheckIPCVisitor. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... File tools/clang/plugins/FindBadConstructsConsumer.h (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Fi... tools/clang/plugins/FindBadConstructsConsumer.h:49: void Visit(clang::ASTContext& context); On 2016/03/07 23:44:39, dcheng wrote: > Nit: Traverse() for consistency with RecursiveASTVisitor's naming scheme. Done. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... File tools/clang/plugins/tests/ipc.cpp (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.cpp:126: // NOT CHECKED: WriteParam() can only be used in ParamTraits templates On 2016/03/07 23:44:39, dcheng wrote: > I don't think I understand how this comment relates to 127-129: 128 is still an > error with the ipc checker. Done. https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.cpp:234: IPC::WriteParam(nullptr, p + 1); // ERROR On 2016/03/07 23:44:39, dcheng wrote: > Slightly unexpected. I guess this basically shouldn't come up at all though. Yes, p + 1 turns uint64_t into its base type, which is (blacklisted) unsigned long. The workaround is to add an explicit cast to a non-blacklisted type.
https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... File tools/clang/plugins/CheckIPCVisitor.cpp (right): https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... tools/clang/plugins/CheckIPCVisitor.cpp:119: if (auto* cast_expr = dyn_cast<ImplicitCastExpr>(arg_type_expr)) { On 2016/03/08 at 01:16:10, Dmitry Skiba wrote: > On 2016/03/07 23:44:39, dcheng wrote: > > Nit: > > > > } else if {auto* cast_expr = dyn_cast<ImplicitCastExr>(...)) { > > arg_type_expr = cast_expr->getSubExpr(); > > } else { > > arg_type = arg_type_expr->getType(); > > } > > Hmm, but in original code arg_type assignment happens in both branches. Oh I see. How about just doing this instead then: arg_expr = arg_expr->IgnoreImplicit(); // Note: this is reusing the param, but that's perfectly legal here. if (auto* cast_expr = dyn_cast<ECE>(arg_expr)) { arg_type = cast_expr->getTypeAsWritten(); } else { arg_type = arg_expr->getType(); } Then we can skip having to handle a lot of implicit AST nodes ourselves: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/llvm/t...
On 2016/03/08 01:30:32, dcheng wrote: > https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... > File tools/clang/plugins/CheckIPCVisitor.cpp (right): > > https://codereview.chromium.org/1665363002/diff/340001/tools/clang/plugins/Ch... > tools/clang/plugins/CheckIPCVisitor.cpp:119: if (auto* cast_expr = > dyn_cast<ImplicitCastExpr>(arg_type_expr)) { > On 2016/03/08 at 01:16:10, Dmitry Skiba wrote: > > On 2016/03/07 23:44:39, dcheng wrote: > > > Nit: > > > > > > } else if {auto* cast_expr = dyn_cast<ImplicitCastExr>(...)) { > > > arg_type_expr = cast_expr->getSubExpr(); > > > } else { > > > arg_type = arg_type_expr->getType(); > > > } > > > > Hmm, but in original code arg_type assignment happens in both branches. > > Oh I see. How about just doing this instead then: > > arg_expr = arg_expr->IgnoreImplicit(); // Note: this is reusing the param, but > that's perfectly legal here. > if (auto* cast_expr = dyn_cast<ECE>(arg_expr)) { > arg_type = cast_expr->getTypeAsWritten(); > } else { > arg_type = arg_expr->getType(); > } > > Then we can skip having to handle a lot of implicit AST nodes ourselves: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/llvm/t... Thanks for IgnoreImplicit()! It really simplifies the whole thing.
lgtm Btw, I poked at the template arg thing... we might have a bit of hope, but I need to experiment with it more: it's not quite as precise as if the template were explicitly instantiated, but maybe it's good enough. Either way, let's land this and it'll be easier to experiment from there. Also, what's the guidance if you do need to write something that's size_t sized, e.g. vector::size()? Should people just cast these to uint64_t?
On 2016/03/08 18:00:47, dcheng wrote: > lgtm > > Btw, I poked at the template arg thing... we might have a bit of hope, but I > need to experiment with it more: it's not quite as precise as if the template > were explicitly instantiated, but maybe it's good enough. Either way, let's land > this and it'll be easier to experiment from there. That's good news! > Also, what's the guidance if you do need to write something that's size_t sized, > e.g. vector::size()? Should people just cast these to uint64_t? Yes, cast to some non-blacklisted type. Actually all code that is currently doing that is casting to plain int, see ParamTraits<> for std::vector, std::set, etc.
The CQ bit was checked by dskiba@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1665363002/#ps400001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665363002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665363002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dskiba@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1665363002/#ps420001 (title: "rebase to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1665363002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1665363002/420001
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Clang plugin to check that unstable types are not used in IPC. We want IPC messages to be stable so that 32-bit and 64-bit processes can talk to each other. This plugin blacklists the following types that are known to be unstable: 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) Blacklisted types are not allowed in IPC::WriteParam() invocations or IPC::CheckedTuple<> specializations. BUG=581409 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Clang plugin to check that unstable types are not used in IPC. We want IPC messages to be stable so that 32-bit and 64-bit processes can talk to each other. This plugin blacklists the following types that are known to be unstable: 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) Blacklisted types are not allowed in IPC::WriteParam() invocations or IPC::CheckedTuple<> specializations. BUG=581409 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/a817c371a7a33cdf9282b711dafbdcee83b788e4 Cr-Commit-Position: refs/heads/master@{#380151} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/a817c371a7a33cdf9282b711dafbdcee83b788e4 Cr-Commit-Position: refs/heads/master@{#380151}
Message was sent while issue was closed.
hans@chromium.org changed reviewers: + hans@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... File tools/clang/plugins/tests/ipc.txt (right): https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... tools/clang/plugins/tests/ipc.txt:5: WriteParam(pickle, static_cast<long>(container.value)); // ERROR For some reason, this one (might be more, I haven't diffed properly yet) doesn't get emitted on Windows. First I thought it was an -fdelayed-template-parsing thing, but it doesn't show up at all. This means the test is not passing on Windows, which blocks Clang rolls.
Message was sent while issue was closed.
On 2016/03/09 22:00:07, hans wrote: > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > File tools/clang/plugins/tests/ipc.txt (right): > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > tools/clang/plugins/tests/ipc.txt:5: WriteParam(pickle, > static_cast<long>(container.value)); // ERROR > For some reason, this one (might be more, I haven't diffed properly yet) doesn't > get emitted on Windows. > > First I thought it was an -fdelayed-template-parsing thing, but it doesn't show > up at all. > > This means the test is not passing on Windows, which blocks Clang rolls. Sorry about that, let me fix it. Do you know which bot should I trigger to test my fix?
Message was sent while issue was closed.
On 2016/03/09 at 22:06:22, dskiba wrote: > On 2016/03/09 22:00:07, hans wrote: > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > File tools/clang/plugins/tests/ipc.txt (right): > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > tools/clang/plugins/tests/ipc.txt:5: WriteParam(pickle, > > static_cast<long>(container.value)); // ERROR > > For some reason, this one (might be more, I haven't diffed properly yet) doesn't > > get emitted on Windows. > > > > First I thought it was an -fdelayed-template-parsing thing, but it doesn't show > > up at all. > > > > This means the test is not passing on Windows, which blocks Clang rolls. > > Sorry about that, let me fix it. Do you know which bot should I trigger to test my fix? You'll have to test locally, I don't think clang tot is covered by any of the CQ builders.
Message was sent while issue was closed.
On 2016/03/09 22:08:32, dcheng wrote: > On 2016/03/09 at 22:06:22, dskiba wrote: > > On 2016/03/09 22:00:07, hans wrote: > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > File tools/clang/plugins/tests/ipc.txt (right): > > > > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > tools/clang/plugins/tests/ipc.txt:5: WriteParam(pickle, > > > static_cast<long>(container.value)); // ERROR > > > For some reason, this one (might be more, I haven't diffed properly yet) > doesn't > > > get emitted on Windows. > > > > > > First I thought it was an -fdelayed-template-parsing thing, but it doesn't > show > > > up at all. > > > > > > This means the test is not passing on Windows, which blocks Clang rolls. > > > > Sorry about that, let me fix it. Do you know which bot should I trigger to > test my fix? > > You'll have to test locally, I don't think clang tot is covered by any of the CQ > builders. Happy to try things for you if you don't have a Windows machine handy.
Message was sent while issue was closed.
Can we revert until then to unlock the roll? On Mar 9, 2016 5:09 PM, <hans@chromium.org> wrote: > On 2016/03/09 22:08:32, dcheng wrote: > > On 2016/03/09 at 22:06:22, dskiba wrote: > > > On 2016/03/09 22:00:07, hans wrote: > > > > > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > > File tools/clang/plugins/tests/ipc.txt (right): > > > > > > > > > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > > tools/clang/plugins/tests/ipc.txt:5: WriteParam(pickle, > > > > static_cast<long>(container.value)); // ERROR > > > > For some reason, this one (might be more, I haven't diffed properly > yet) > > doesn't > > > > get emitted on Windows. > > > > > > > > First I thought it was an -fdelayed-template-parsing thing, but it > doesn't > > show > > > > up at all. > > > > > > > > This means the test is not passing on Windows, which blocks Clang > rolls. > > > > > > Sorry about that, let me fix it. Do you know which bot should I > trigger to > > test my fix? > > > > You'll have to test locally, I don't think clang tot is covered by any > of the > CQ > > builders. > > Happy to try things for you if you don't have a Windows machine handy. > > https://codereview.chromium.org/1665363002/ > -- 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.
Message was sent while issue was closed.
On 2016/03/09 22:49:55, Nico wrote: > Can we revert until then to unlock the roll? > On Mar 9, 2016 5:09 PM, <mailto:hans@chromium.org> wrote: > > > On 2016/03/09 22:08:32, dcheng wrote: > > > On 2016/03/09 at 22:06:22, dskiba wrote: > > > > On 2016/03/09 22:00:07, hans wrote: > > > > > > > > > > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > > > File tools/clang/plugins/tests/ipc.txt (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > > > tools/clang/plugins/tests/ipc.txt:5: WriteParam(pickle, > > > > > static_cast<long>(container.value)); // ERROR > > > > > For some reason, this one (might be more, I haven't diffed properly > > yet) > > > doesn't > > > > > get emitted on Windows. > > > > > > > > > > First I thought it was an -fdelayed-template-parsing thing, but it > > doesn't > > > show > > > > > up at all. > > > > > > > > > > This means the test is not passing on Windows, which blocks Clang > > rolls. > > > > > > > > Sorry about that, let me fix it. Do you know which bot should I > > trigger to > > > test my fix? > > > > > > You'll have to test locally, I don't think clang tot is covered by any > > of the > > CQ > > > builders. > > > > Happy to try things for you if you don't have a Windows machine handy. > > > > https://codereview.chromium.org/1665363002/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org. I'm preparing a fix, but revert is fine too.
Message was sent while issue was closed.
On 2016/03/09 22:54:06, Dmitry Skiba wrote: > On 2016/03/09 22:49:55, Nico wrote: > > Can we revert until then to unlock the roll? > > On Mar 9, 2016 5:09 PM, <mailto:hans@chromium.org> wrote: > > > > > On 2016/03/09 22:08:32, dcheng wrote: > > > > On 2016/03/09 at 22:06:22, dskiba wrote: > > > > > On 2016/03/09 22:00:07, hans wrote: > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > > > > File tools/clang/plugins/tests/ipc.txt (right): > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > > > > tools/clang/plugins/tests/ipc.txt:5: WriteParam(pickle, > > > > > > static_cast<long>(container.value)); // ERROR > > > > > > For some reason, this one (might be more, I haven't diffed properly > > > yet) > > > > doesn't > > > > > > get emitted on Windows. > > > > > > > > > > > > First I thought it was an -fdelayed-template-parsing thing, but it > > > doesn't > > > > show > > > > > > up at all. > > > > > > > > > > > > This means the test is not passing on Windows, which blocks Clang > > > rolls. > > > > > > > > > > Sorry about that, let me fix it. Do you know which bot should I > > > trigger to > > > > test my fix? > > > > > > > > You'll have to test locally, I don't think clang tot is covered by any > > > of the > > > CQ > > > > builders. > > > > > > Happy to try things for you if you don't have a Windows machine handy. > > > > > > https://codereview.chromium.org/1665363002/ > > > > > > > -- > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > I'm preparing a fix, but revert is fine too. Just 5 more minutes.
Message was sent while issue was closed.
On 2016/03/09 22:55:43, Dmitry Skiba wrote: > On 2016/03/09 22:54:06, Dmitry Skiba wrote: > > On 2016/03/09 22:49:55, Nico wrote: > > > Can we revert until then to unlock the roll? > > > On Mar 9, 2016 5:09 PM, <mailto:hans@chromium.org> wrote: > > > > > > > On 2016/03/09 22:08:32, dcheng wrote: > > > > > On 2016/03/09 at 22:06:22, dskiba wrote: > > > > > > On 2016/03/09 22:00:07, hans wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > > > > > File tools/clang/plugins/tests/ipc.txt (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1665363002/diff/420001/tools/clang/plugins/te... > > > > > > > tools/clang/plugins/tests/ipc.txt:5: WriteParam(pickle, > > > > > > > static_cast<long>(container.value)); // ERROR > > > > > > > For some reason, this one (might be more, I haven't diffed properly > > > > yet) > > > > > doesn't > > > > > > > get emitted on Windows. > > > > > > > > > > > > > > First I thought it was an -fdelayed-template-parsing thing, but it > > > > doesn't > > > > > show > > > > > > > up at all. > > > > > > > > > > > > > > This means the test is not passing on Windows, which blocks Clang > > > > rolls. > > > > > > > > > > > > Sorry about that, let me fix it. Do you know which bot should I > > > > trigger to > > > > > test my fix? > > > > > > > > > > You'll have to test locally, I don't think clang tot is covered by any > > > > of the > > > > CQ > > > > > builders. > > > > > > > > Happy to try things for you if you don't have a Windows machine handy. > > > > > > > > https://codereview.chromium.org/1665363002/ > > > > > > > > > > -- > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > > I'm preparing a fix, but revert is fine too. > > Just 5 more minutes. Windows fix: https://codereview.chromium.org/1785523002/
Message was sent while issue was closed.
A revert of this CL (patchset #22 id:420001) has been created in https://codereview.chromium.org/1783803002/ by thakis@chromium.org. The reason for reverting is: IPC plugin tests are failing on the clang/tot win bots, even after the fix attempt..
Message was sent while issue was closed.
Oops, this is the old CL. I'll created a new one and reupload latest change there. |
