Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2911633002:
Expanding traffic_annotation_extractor clang tool to extract network calls. (Closed)
|
Created:
3 years, 6 months ago by Ramin Halavati Modified:
3 years, 6 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpanding traffic_annotation_extractor clang tool to extract network calls.
To implement presubmit and unittest checks for network traffic annotations,
traffic_annotation_extractor clang tool is expanded to find and report calls to
network request function that require annotations.
BUG=656607
Review-Url: https://codereview.chromium.org/2911633002
Cr-Commit-Position: refs/heads/master@{#475426}
Committed: https://chromium.googlesource.com/chromium/src/+/46e2d63b6f5aaa82d170338f4571cffb1505b5f8
Patch Set 1 #
Total comments: 6
Patch Set 2 : Clang tool updated. #
Total comments: 4
Patch Set 3 : Clang tool updated. #
Total comments: 22
Patch Set 4 : Comments addressed. #
Messages
Total messages: 17 (8 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
rhalavati@chromium.org changed reviewers: + dcheng@chromium.org
Hi Daniel, I've updated traffic_annotation_extractor clang tool to find network request functions that need network traffic annotations. Please review it.
Caveats: I'm basically assuming the code works as advertised. Might be good to have some tests checked in at some point so it's easier to understand the impact on output. https://codereview.chromium.org/2911633002/diff/40001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2911633002/diff/40001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:105: CallInstance() : has_annotation(false) {} Or just use an in-class initializer to be consistent. I don't feel strongly though. https://codereview.chromium.org/2911633002/diff/40001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:162: // Trim leading "../"s from file path. Nit: this comment probably before line 165, or it should cover 163-164 as well. https://codereview.chromium.org/2911633002/diff/40001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:187: // couldn't do it. hasAnyParameter(hasType(recordDecl(anyOf(hasName("net::NetworkTrafficAnnotationTag"), hasName("net::PartialNetworkTrafficAnnotationTag"))))) would be my untested guess for using a matcher. You can use the same anyOf() trick used to capture the function context to make this optionally set.
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you very much Daniel, comment addressed. About adding tests, to run the tool on them I need to have a compiled build_directory. Should I add them to src/BUILD.gn or there is another way to work around this? The best approach I thought about is to have a BUILD.gn in my_tests folder, run GN on it in each test, compile it, and then run tool and see if the results are OK. Otherwise I should pollute src/BUILD.gn with it. Martin, Please review auditor.py. The new outputs are just ignored in this CL, but the next CL on top of this uses them for presubmit and unittest checks. https://codereview.chromium.org/2911633002/diff/40001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2911633002/diff/40001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:105: CallInstance() : has_annotation(false) {} On 2017/05/27 08:27:46, dcheng wrote: > Or just use an in-class initializer to be consistent. I don't feel strongly > though. Done. https://codereview.chromium.org/2911633002/diff/40001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:162: // Trim leading "../"s from file path. On 2017/05/27 08:27:46, dcheng wrote: > Nit: this comment probably before line 165, or it should cover 163-164 as well. Done. https://codereview.chromium.org/2911633002/diff/40001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:187: // couldn't do it. On 2017/05/27 08:27:46, dcheng wrote: > hasAnyParameter(hasType(recordDecl(anyOf(hasName("net::NetworkTrafficAnnotationTag"), > hasName("net::PartialNetworkTrafficAnnotationTag"))))) would be my untested > guess for using a matcher. > > You can use the same anyOf() trick used to capture the function context to make > this optionally set. Thank you very much.
LGTM On 2017/05/29 08:13:28, Ramin Halavati wrote: > Thank you very much Daniel, comment addressed. > > About adding tests, to run the tool on them I need to have a compiled > build_directory. Should I add them to src/BUILD.gn or there is another way to > work around this? > The best approach I thought about is to have a BUILD.gn in my_tests folder, run > GN on it in each test, compile it, and then run tool and see if the results are > OK. Otherwise I should pollute src/BUILD.gn with it. Regarding testing, test_tool.py automatically generates the compile DB, so there's no need to have a BUILD.gn--however, it's not a great fit since it expects the tool to output replacements. Instead, we can just write a custom compile_commands.json here and use self-contained test files. In this case, network_traffic_annotation.h is simple enough that we can likely just include it directly and stub out base/logging.h so it will compile. But we can do this in followups. https://codereview.chromium.org/2911633002/diff/60001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2911633002/diff/60001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:182: (result.Nodes.getNodeAs<clang::RecordDecl>("annotation") != NULL); Nit: nullptr instead of NULL https://codereview.chromium.org/2911633002/diff/60001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:310: unless(hasAncestor(functionDecl())))) One thing to consider is to move some of these expressions to a submatcher that's reused. e.g. auto is_network_request_function = callExpr( hasDeclaration(functionDecl(...)); auto bind_function_context_if_present = expr(anyOf( hasAncestor( functionDecl().bind("function_context")), unless(...)); auto has_annotation_parameter = callExpr( anyOf(hasAnyParameter(...)), unless(...)); then combine it with: match_finder.addMatcher( callExpr( is_network_request_function, has_annotation_parameter, bind_function_context_if_present), <callback>);
Thank you Daniel, comments addressed. Is compile_commands.json the only requirement to run the clang tool on one file? https://codereview.chromium.org/2911633002/diff/60001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2911633002/diff/60001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:182: (result.Nodes.getNodeAs<clang::RecordDecl>("annotation") != NULL); On 2017/05/29 08:28:00, dcheng wrote: > Nit: nullptr instead of NULL Done. https://codereview.chromium.org/2911633002/diff/60001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:310: unless(hasAncestor(functionDecl())))) On 2017/05/29 08:28:00, dcheng wrote: > One thing to consider is to move some of these expressions to a submatcher > that's reused. > > e.g. > > auto is_network_request_function = callExpr( > hasDeclaration(functionDecl(...)); > > auto bind_function_context_if_present = > expr(anyOf( > hasAncestor( > functionDecl().bind("function_context")), > unless(...)); > > auto has_annotation_parameter = callExpr( > anyOf(hasAnyParameter(...)), unless(...)); > > then combine it with: > match_finder.addMatcher( > callExpr( > is_network_request_function, > has_annotation_parameter, > bind_function_context_if_present), <callback>); Thank you very much, Done.
The usual disclaimer that I'm not a native English speaker, but I added some suggestions about missing articles :) LGTM when comments are addressed. Thanks! https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/README.md:35: - Line 3: Name of the function in which annotation is defined. nit: the annotation https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/README.md:36: - Line 4: Line number of annotation. nit: the annotation https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/README.md:48: - Line 4: Name of called function. nit: of the https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/README.md:49: - Line 5: Does the call have annotation? nit: an annotation https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:11: // them to llvm::outs. It also extracts all calls the following network request typo: calls of https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... File tools/traffic_annotation/auditor/traffic_annotation_auditor.py (right): https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:77: raw_annotations: str Serialization of annotations and metadata. Each Please update the function doc to mention calls as well. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:106: while current < len(lines) - 1: Not necessarily in this CL, but this logic looks like a good candidate for a unittest. We can have a similar setup as in our team's automation scripts. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:108: if current + 5 >= len(lines): +5 is not enough, since you're accessing +6 at line 120. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:127: current += 1 optional: Consider moving this current += 1 at the end of the body of the large while statement? I think it's a bit more readable when we just make this +1 step to the next row at the end rather than always having to do [-1]. Possibly also add a comment why is that += 1 there. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:135: Should we handle an empty |annotation_text| here? https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:149: errors.append("Annotation in file '%s', line %i, has error: %s" % nit: an error
Thank you Martin, all comments addressed. https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/README.md (right): https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/README.md:35: - Line 3: Name of the function in which annotation is defined. On 2017/05/29 21:03:36, msramek wrote: > nit: the annotation Done. https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/README.md:36: - Line 4: Line number of annotation. On 2017/05/29 21:03:36, msramek wrote: > nit: the annotation Done. https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/README.md:48: - Line 4: Name of called function. On 2017/05/29 21:03:36, msramek wrote: > nit: of the Done. https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/README.md:49: - Line 5: Does the call have annotation? On 2017/05/29 21:03:36, msramek wrote: > nit: an annotation Done. https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... File tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp (right): https://codereview.chromium.org/2911633002/diff/80001/tools/clang/traffic_ann... tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp:11: // them to llvm::outs. It also extracts all calls the following network request On 2017/05/29 21:03:36, msramek wrote: > typo: calls of Done. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... File tools/traffic_annotation/auditor/traffic_annotation_auditor.py (right): https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:77: raw_annotations: str Serialization of annotations and metadata. Each On 2017/05/29 21:03:37, msramek wrote: > Please update the function doc to mention calls as well. Done. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:106: while current < len(lines) - 1: On 2017/05/29 21:03:37, msramek wrote: > Not necessarily in this CL, but this logic looks like a good candidate for a > unittest. We can have a similar setup as in our team's automation scripts. Acknowledged. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:108: if current + 5 >= len(lines): On 2017/05/29 21:03:36, msramek wrote: > +5 is not enough, since you're accessing +6 at line 120. Done. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:127: current += 1 On 2017/05/29 21:03:37, msramek wrote: > optional: Consider moving this current += 1 at the end of the body of the large > while statement? I think it's a bit more readable when we just make this +1 step > to the next row at the end rather than always having to do [-1]. Possibly also > add a comment why is that += 1 there. Done. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:135: On 2017/05/29 21:03:37, msramek wrote: > Should we handle an empty |annotation_text| here? It would be checked later when proto is deserialized. I will add it to unittest so that it ensures that it's tested somewhere. https://codereview.chromium.org/2911633002/diff/80001/tools/traffic_annotatio... tools/traffic_annotation/auditor/traffic_annotation_auditor.py:149: errors.append("Annotation in file '%s', line %i, has error: %s" % On 2017/05/29 21:03:37, msramek wrote: > nit: an error Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2911633002/#ps100001 (title: "Comments addressed.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496119242416840,
"parent_rev": "51f2415dd7e2008cb232c11ccc1bc5cdb79334c0", "commit_rev":
"46e2d63b6f5aaa82d170338f4571cffb1505b5f8"}
Message was sent while issue was closed.
Description was changed from ========== Expanding traffic_annotation_extractor clang tool to extract network calls. To implement presubmit and unittest checks for network traffic annotations, traffic_annotation_extractor clang tool is expanded to find and report calls to network request function that require annotations. BUG=656607 ========== to ========== Expanding traffic_annotation_extractor clang tool to extract network calls. To implement presubmit and unittest checks for network traffic annotations, traffic_annotation_extractor clang tool is expanded to find and report calls to network request function that require annotations. BUG=656607 Review-Url: https://codereview.chromium.org/2911633002 Cr-Commit-Position: refs/heads/master@{#475426} Committed: https://chromium.googlesource.com/chromium/src/+/46e2d63b6f5aaa82d170338f4571... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/46e2d63b6f5aaa82d170338f4571... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
