Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shipit_static_analysis: Don't report defects expanded from macros #658

Closed
nnethercote opened this issue Oct 6, 2017 · 10 comments

Comments

Projects
None yet
5 participants
@nnethercote
Copy link

commented Oct 6, 2017

MozReview URL: https://reviewboard.mozilla.org/r/187072/#review192094
Problem: It thinks gtest TEST macros are trivial constructors, and wants me to put = default on them. I'm not sure what the TEST macros look like when they are expanded, perhaps the reasoning here is obvious once you work that out.

@glandium

This comment has been minimized.

Copy link

commented Oct 6, 2017

One way to look at the problem is that static analysis shouldn't warn about problems that are the result from macro expansion of macros that haven't been touched in a patch.

There might be legitimate problems with the TEST macro, but they're not relevant to the changes in that mozreview url (plus, gtest is third-party code).

@squelart

This comment has been minimized.

Copy link

commented Oct 6, 2017

Got the same+similar problems with https://reviewboard.mozilla.org/r/180768/#review192120 :

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]
Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

@jankeromnes

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

Thank you for reporting this issue! I agree it isn't fair that our bot complains about code defects in expanded macros (just because you're using a macro that can be improved doesn't mean your patch should be blocked until the macro is fixed).

I think we should find a way to detect when code defects are expanded from macros, and not block the patch if that's the case (maybe we should still post a comment, or maybe we should ignore that defect entirely). Previously we've tried to fix macro-related issues as fast as possible (as they add significant noise to static analysis feedback), but there must be a more reasonable solution to this. EDIT: Especially because macros can come from third-party code, as pointed out by @glandium.

Moreover, the way or bot currently reports such issues is confusing, because it doesn't show where the code defect actually is. We could use the "Notes" generated by clang-tidy to make this a bit clearer (e.g. in the case of the TEST macro):

Note: expanded from macro 'TEST'
Location: obj-x86_64-pc-linux-gnu/dist/include/gtest/gtest.h:2187:42

# define TEST(test_case_name, test_name) GTEST_TEST(test_case_name, test_name)
                                         ^

Note: expanded from macro 'GTEST_TEST'
Location: obj-x86_64-pc-linux-gnu/dist/include/gtest/gtest.h:2181:3

GTEST_TEST_(test_case_name, test_name, \
^

Note: expanded from macro 'GTESTTEST'
Location: obj-x86_64-pc-linux-gnu/dist/include/gtest/internal/gtest-internal.h:1217:55

GTEST_TEST_CLASS_NAME_(test_case_name, test_name)() {}\
                                                    ^
@jankeromnes

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2017

Update: We've elected to simply ignore defects when their first clang-tidy note contains the string "expanded from macro". It's not very clean, but seems to be a robust heuristic to silence code defects expanded from macros that weren't touched by the developer. It's the best approach we see, but please let us know if you have other suggestions.

We hope to have a fix in production next week. Apologies for the bugmail noise in the meantime.

@jankeromnes jankeromnes changed the title shipit_static_analysis: Problem with an automated review shipit_static_analysis: Don't report defects expanded from macros Oct 6, 2017

@La0 La0 self-assigned this Oct 6, 2017

@glandium

This comment has been minimized.

Copy link

commented Oct 6, 2017

What about macros you did change?

La0 added a commit to La0/mozilla-relengapi that referenced this issue Oct 11, 2017

@La0 La0 closed this in #668 Oct 16, 2017

La0 added a commit that referenced this issue Oct 16, 2017

@jankeromnes

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2017

What about macros you did change?

We ignore any defects in code that is expanded from a macro. However if a defect is caught inside a macro, on a line you happened to modify in your patch, then it should be reported. The question is if clang-tidy is able to find defects in unexpanded macros directly.

@glandium Would you be interested in submitting a fake patch to MozReview that purposefully adds a code defect within a macro, in order to verify that we catch these? (You could use Bug 1387052 for that, and there are good defect examples here)

@glandium

This comment has been minimized.

@jankeromnes

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

Perfect, thanks!

@La0 I couldn't find a debug email from clangbot about this push, could you please have a look in the logs to see what happened?

@La0

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2017

@jankeromnes The bot ran through the new diffset and did not find any bug, hence no emails.

Here is the log in staging & production

@jankeromnes

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

@glandium Thanks again for your test push. It showed us that clangbot is unable to detect new defects in macros, and that we'll have to find another way to identify and fix those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.