1615245 - "fopen-usage" checker should ignore gtest include files

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Masatoshi Kimura [:emk]]

gtest-port.h spams "fopen-usage" check failure here:
https://searchfox.org/mozilla-central/rev/1db5ef59eba65d32d6a29a494e87b6078453e559/testing/gtest/gtest/include/gtest/internal/gtest-port.h#2517
even though testing/gtest/gtest/ is a third-party path:
https://searchfox.org/mozilla-central/rev/1db5ef59eba65d32d6a29a494e87b6078453e559/tools/rewriting/ThirdPartyPaths.txt#129

A sample warning message:

55:53.93 e:/m/mozilla-unified/obj-x86_64-pc-mingw32/dist/include\gtest/internal/gtest-port.h(2517,10): note: On Windows executed functions: fopen, fopen_s, open, _open, _sopen, _sopen_s, OpenFile, CreateFileA should never be used due to lossy conversion from UTF8 to ANSI.

Comment 1

[Masatoshi Kimura [:emk]]

Adding dist/include/gtest/internal/ to ThirdPartyPaths.txt fixed the issue.

Comment 2

[Andi [:andi]]

Are we sure we want to exclude that from formatting also?

Comment 3

[Masatoshi Kimura [:emk]]

Yes, because they are third-party code. We only import them from upstream.

Comment 4

[Andi [:andi]]

What I was saying is that we shouldn't include this dist/include/gtest/internal/ into our exclusion, since this is not a path from the tree but a path from the obj directory.

Comment 5

[Masatoshi Kimura [:emk]]

We don't have to care about formatting anyway because we will not edit files under objdir manually and they are not in the repository.

Comment 6

[Andi [:andi]]

True but we shouldn't add files from build directory to the ThirdPartyPaths.txt.

Comment 7

[Masatoshi Kimura [:emk]]

So how can we exclude them from static analysis?

Comment 8

[Andi [:andi]]

For start we know that most of the files from the obj/include are in fact symlinks to the appropriate files from the source so we should check if that file is a symlink and if so use real_path.

Comment 9

[Masatoshi Kimura [:emk]]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #8)

For start we know that most of the files from the obj/include are in fact symlinks

It may not hold on Windows (if objdir is located on FAT32/exFAT voume.) I'm intentionally using this configuration to catch filesystem dependent problems (such as bug 1496050).

Comment 10

[Masatoshi Kimura [:emk]]

I think we can generate a path mapping from <objdir>/install_dist_include.track.

Comment 11

[Masatoshi Kimura [:emk]]

_build_manifests/install/dist_include would be better. python/mozbuild/mozpack/manifests.py is the parser for this file.

Comment 12

[Masatoshi Kimura [:emk]]

How about ignoring gtest namespaces for fopen-usage checker? (like implicit-constructor or explicit-operator-bool) It would be much faster.

Comment 13

[Masatoshi Kimura [:emk]]

Or whitelisting "gtest-port.h" is OK (like nan-expr checker.)

Comment 14

[Andi [:andi]]

(In reply to Masatoshi Kimura [:emk] from comment #13)

Or whitelisting "gtest-port.h" is OK (like nan-expr checker.)

I think this is the path that we should go, are there any other files that should be added to this exception?

Comment 15

[Masatoshi Kimura [:emk]]

gtest-port.h is the only file that we need the exception for.
https://searchfox.org/mozilla-central/search?q=open&case=true&regexp=false&path=include%2Fgtest
https://searchfox.org/mozilla-central/search?q=OpenFile&case=false&regexp=false&path=include%2Fgtest
https://searchfox.org/mozilla-central/search?q=CreateFileA&case=false&regexp=false&path=include%2Fgtest
I also skimmed through the build log.

Comment 16

[Andi [:andi]]

:emk Thanks for this, will push a patch shortly.

Comment 17

[Andi [:andi]]

Attachment: Bug 1615245 - checker `fopen-usage` should ignore some `gtest` include files. r=froydnj

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e56af64a3199
checker `fopen-usage` should ignore some `gtest` include files. r=froydnj

Comment 19

[Razvan Maries]

Backed out for build bustages on CustomMatchers.h

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=289083803&resultStatus=testfailed%2Cbusted%2Cexception&revision=e56af64a3199b70f0c80ea025e337bce40283ffb

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=289083803&repo=autoland&lineNumber=781

Backout: https://hg.mozilla.org/integration/autoland/rev/845fc7e6b7e9074d9ff98d04ac8c9919634e3e8c

Comment 20

[Andi [:andi]]

This will solve it. Thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a63e7e8cd4d
checker `fopen-usage` should ignore some `gtest` include files. r=froydnj

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5a63e7e8cd4d