Closed Bug 1614434 Opened 5 years ago Closed 5 years ago

function nBuildCache(LookupCacheV2* cache, const _PrefixArray& aPrefixArray) is not used

Categories

(Toolkit :: Safe Browsing, defect, P3)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: Sylvestre, Assigned: zech.ph, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=C++])

Attachments

(3 files, 1 obsolete file)

log.txt
3.78 KB, text/plain
Details
Bug 1614434 - Remove unsused function BuildCache. r=gcp
47 bytes, text/x-phabricator-request
Details | Review
log.txt
3.85 KB, text/plain
Details

Filling as a good first bug to learn workflows.

The function nBuildCache(LookupCacheV2* cache, const _PrefixArray& aPrefixArray) is not used. We know as it is declared as static.
https://searchfox.org/mozilla-central/source/toolkit/components/url-classifier/tests/gtest/Common.cpp#195-205

As the change is trivial, it is just to learn how to contribute to Firefox.

Found by https://clang.llvm.org/docs/DiagnosticsReference.html#wunused-function

Tutorial to contribute:
https://firefox-source-docs.mozilla.org/tools/docs/contribute/how_to_contribute_firefox.html

Please don't ask for the bug to be assigned. It will be automatically assigned to the first patch.

Assignee: nobody → mbansal
Status: NEW → ASSIGNED
Attachment #9125839 - Attachment is obsolete: true

Bug unassigned as Mahak already fixed a few good first bug.
I would like to keep them open for other new comers!

Assignee: mbansal → nobody
Status: ASSIGNED → NEW

May I again?

Well, I'm actually already on it :)

Attached file log.txt —

Grepping for nBuildCache yields no such function declaration, but there exists one called BuildCache at the specified location in the mentioned file - so I assume it is the function in question. So I went ahead and removed that function, however, then the build (Linux Desktop) fails (see attached build log). Did I eventually check the correct function? And if yes, shall I continue and check whether the function really could be removed or do you want to proceed any other way?

I also removed RefPtr<T> SetupLookupCache(const _PrefixArray& aPrefixArray) (https://searchfox.org/mozilla-central/source/toolkit/components/url-classifier/tests/gtest/Common.cpp#229). Now everything complies again and tests pass, so I guess I'll submit a patch.

Removed unused function BuildCache(LookupCacheV2* cache, const _PrefixArray& aPrefixArray). This also required removal of RefPtr<T> SetupLookupCache(const _PrefixArray& aPrefixArray) (formerly line 229).

Assignee: nobody → zech.ph
Status: NEW → ASSIGNED
Priority: -- → P3
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5fdf628512a0 Remove unsused function BuildCache. r=gcp

Sorry for my excuse, but I'm pretty new here, so I am not sure what info you need?

Flags: needinfo?(zech.ph)

If you look at the Failure log, you will see a link error.
Seems that some tests were using this code. You could disable the test:

[task 2020-02-20T14:48:14.241Z] 14:48:14 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/build'
[task 2020-02-20T14:48:14.242Z] 14:48:14 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 -U_FORTIFY_SOURCE -fno-common -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fsanitize=bool,bounds,integer-divide-by-zero,object-size,pointer-overflow,return,vla-bound -fno-sanitize-recover=bool,bounds,integer-divide-by-zero,object-size,pointer-overflow,return,vla-bound -fsanitize-blacklist=/builds/worker/workspace/build/src/obj-firefox/ubsan_blacklist.txt -fsanitize=address -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -gline-tables-only -fno-omit-frame-pointer -funwind-tables -Werror -fPIC -shared -Wl,--gc-sections -Wl,-h,libxul.so -o libxul.so /builds/worker/workspace/build/src/obj-firefox/toolkit/library/build/libxul_so.list -lpthread -fsanitize=undefined -rdynamic -fsanitize=address -rdynamic -Wl,-z,noexecstack -Wl,-z,text -Wl,-z,relro -Wl,-z,nocopyreloc -Wl,-Bsymbolic-functions -Wl,--build-id=sha1 -Wl,-Bsymbolic -Wl,-rpath-link,/builds/worker/workspace/build/src/obj-firefox/dist/bin -Wl,-rpath-link,/usr/local/lib ../../../security/nss/lib/crmf/crmf_crmf/libcrmf.a ../../../js/src/build/libjs_static.a /builds/worker/workspace/build/src/obj-firefox/x86_64-unknown-linux-gnu/release/libgkrust.a ../../../config/external/nspr/pr/libnspr4.so ../../../config/external/nspr/libc/libplc4.so ../../../config/external/nspr/ds/libplds4.so ../../../config/external/lgpllibs/liblgpllibs.so ../../../security/nss/lib/nss/nss_nss3/libnss3.so ../../../security/nss/lib/util/util_nssutil3/libnssutil3.so ../../../security/nss/lib/smime/smime_smime3/libsmime3.so ../../../config/external/sqlite/libmozsqlite3.so ../../../security/nss/lib/ssl/ssl_ssl3/libssl3.so ../../../widget/gtk/mozgtk/stub/libmozgtk_stub.so ../../../widget/gtk/mozwayland/libmozwayland.so -Wl,--version-script,symverscript -lrt -lm -lX11 -lX11-xcb -lxcb -lXcomposite -lXcursor -lXdamage -lXext -lXfixes -lXi -lXrender -lpthread -ldl -lc -lfreetype -lfontconfig -ldbus-glib-1 -ldbus-1 -lgobject-2.0 -lglib-2.0 -latk-1.0 -lpangocairo-1.0 -lgdk_pixbuf-2.0 -lcairo-gobject -lpango-1.0 -lcairo -lgio-2.0 -lxcb-shm -lpangoft2-1.0 -lXt -lgthread-2.0 -Wl,--version-script,/builds/worker/workspace/build/src/build/unix/stdc++compat/hide_std.ld
[task 2020-02-20T14:48:14.242Z] 14:48:14 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/build'
[task 2020-02-20T14:48:17.348Z] 14:48:17 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/gtest'
[task 2020-02-20T14:48:17.348Z] 14:48:17 INFO - /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 -U_FORTIFY_SOURCE -fno-common -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fsanitize=bool,bounds,integer-divide-by-zero,object-size,pointer-overflow,return,vla-bound -fno-sanitize-recover=bool,bounds,integer-divide-by-zero,object-size,pointer-overflow,return,vla-bound -fsanitize-blacklist=/builds/worker/workspace/build/src/obj-firefox/ubsan_blacklist.txt -fsanitize=address -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -gline-tables-only -fno-omit-frame-pointer -funwind-tables -Werror -fPIC -shared -Wl,--gc-sections -Wl,-h,libxul.so -o libxul.so /builds/worker/workspace/build/src/obj-firefox/toolkit/library/gtest/libxul_so.list -lpthread -fsanitize=undefined -rdynamic -fsanitize=address -rdynamic -Wl,-z,noexecstack -Wl,-z,text -Wl,-z,relro -Wl,-z,nocopyreloc -Wl,-Bsymbolic-functions -Wl,--build-id=sha1 -Wl,-Bsymbolic -Wl,-rpath-link,/builds/worker/workspace/build/src/obj-firefox/dist/bin -Wl,-rpath-link,/usr/local/lib ../../../security/nss/lib/crmf/crmf_crmf/libcrmf.a ../../../js/src/build/libjs_static.a /builds/worker/workspace/build/src/obj-firefox/x86_64-unknown-linux-gnu/release/libgkrust_gtest.a ../../../config/external/nspr/pr/libnspr4.so ../../../config/external/nspr/libc/libplc4.so ../../../config/external/nspr/ds/libplds4.so ../../../config/external/lgpllibs/liblgpllibs.so ../../../security/nss/lib/nss/nss_nss3/libnss3.so ../../../security/nss/lib/util/util_nssutil3/libnssutil3.so ../../../security/nss/lib/smime/smime_smime3/libsmime3.so ../../../config/external/sqlite/libmozsqlite3.so ../../../security/nss/lib/ssl/ssl_ssl3/libssl3.so ../../../widget/gtk/mozgtk/stub/libmozgtk_stub.so ../../../widget/gtk/mozwayland/libmozwayland.so -Wl,--version-script,symverscript -lrt -lm -lX11 -lX11-xcb -lxcb -lXcomposite -lXcursor -lXdamage -lXext -lXfixes -lXi -lXrender -lpthread -ldl -lc -lfreetype -lfontconfig -ldbus-glib-1 -ldbus-1 -lgobject-2.0 -lglib-2.0 -latk-1.0 -lpangocairo-1.0 -lgdk_pixbuf-2.0 -lcairo-gobject -lpango-1.0 -lcairo -lgio-2.0 -lxcb-shm -lpangoft2-1.0 -lXt -lgthread-2.0 -Wl,--version-script,/builds/worker/workspace/build/src/build/unix/stdc++compat/hide_std.ld
[task 2020-02-20T14:48:17.348Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: ../../components/url-classifier/tests/gtest/Unified_cpp_tests_gtest0.o: in function void TestCache<mozilla::safebrowsing::LookupCacheV2>(mozilla::safebrowsing::SafebrowsingHash<32u, mozilla::safebrowsing::CompletionComparator>, bool, bool, bool, mozilla::safebrowsing::LookupCacheV2*)': [task 2020-02-20T14:48:17.348Z] 14:48:17 INFO - /builds/worker/workspace/build/src/toolkit/components/url-classifier/tests/gtest/TestCaching.cpp:76: undefined reference toRefPtr<mozilla::safebrowsing::LookupCacheV2> SetupLookupCache<mozilla::safebrowsing::LookupCacheV2>(nsTArray<nsTString<char> > const&)'
[task 2020-02-20T14:48:17.348Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: ../../components/url-classifier/tests/gtest/Unified_cpp_tests_gtest0.o: in function void TestCache<mozilla::safebrowsing::LookupCacheV4>(mozilla::safebrowsing::SafebrowsingHash<32u, mozilla::safebrowsing::CompletionComparator>, bool, bool, bool, mozilla::safebrowsing::LookupCacheV4*)': [task 2020-02-20T14:48:17.349Z] 14:48:17 INFO - /builds/worker/workspace/build/src/toolkit/components/url-classifier/tests/gtest/TestCaching.cpp:76: undefined reference toRefPtr<mozilla::safebrowsing::LookupCacheV4> SetupLookupCache<mozilla::safebrowsing::LookupCacheV4>(nsTArray<nsTString<char> > const&)'
[task 2020-02-20T14:48:17.349Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: ../../components/url-classifier/tests/gtest/Unified_cpp_tests_gtest0.o: in function void TestInvalidateExpiredCacheEntry<mozilla::safebrowsing::LookupCacheV2>()': [task 2020-02-20T14:48:17.349Z] 14:48:17 INFO - /builds/worker/workspace/build/src/toolkit/components/url-classifier/tests/gtest/TestCaching.cpp:198: undefined reference toRefPtr<mozilla::safebrowsing::LookupCacheV2> SetupLookupCache<mozilla::safebrowsing::LookupCacheV2>(nsTArray<nsTString<char> > const&)'
[task 2020-02-20T14:48:17.349Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: ../../components/url-classifier/tests/gtest/Unified_cpp_tests_gtest0.o: in function void TestInvalidateExpiredCacheEntry<mozilla::safebrowsing::LookupCacheV4>()': [task 2020-02-20T14:48:17.349Z] 14:48:17 INFO - /builds/worker/workspace/build/src/toolkit/components/url-classifier/tests/gtest/TestCaching.cpp:198: undefined reference toRefPtr<mozilla::safebrowsing::LookupCacheV4> SetupLookupCache<mozilla::safebrowsing::LookupCacheV4>(nsTArray<nsTString<char> > const&)'
[task 2020-02-20T14:48:17.349Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: ../../components/url-classifier/tests/gtest/Unified_cpp_tests_gtest0.o: in function UrlClassifierCaching_NegativeCacheExpireV2_Test::TestBody()': [task 2020-02-20T14:48:17.349Z] 14:48:17 INFO - /builds/worker/workspace/build/src/toolkit/components/url-classifier/tests/gtest/TestCaching.cpp:237: undefined reference toRefPtr<mozilla::safebrowsing::LookupCacheV2> SetupLookupCache<mozilla::safebrowsing::LookupCacheV2>(nsTArray<nsTString<char> > const&)'
[task 2020-02-20T14:48:17.349Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: ../../components/url-classifier/tests/gtest/Unified_cpp_tests_gtest0.o: in function UrlClassifierCaching_NegativeCacheExpireV4_Test::TestBody()': [task 2020-02-20T14:48:17.349Z] 14:48:17 INFO - /builds/worker/workspace/build/src/toolkit/components/url-classifier/tests/gtest/TestCaching.cpp:260: undefined reference toRefPtr<mozilla::safebrowsing::LookupCacheV4> SetupLookupCache<mozilla::safebrowsing::LookupCacheV4>(nsTArray<nsTString<char> > const&)'
[task 2020-02-20T14:48:17.350Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: ../../components/url-classifier/tests/gtest/Unified_cpp_tests_gtest0.o: in function UrlClassifierLookupCacheV4_BuildAPI_Test::TestBody()': [task 2020-02-20T14:48:17.350Z] 14:48:17 INFO - /builds/worker/workspace/build/src/toolkit/components/url-classifier/tests/gtest/TestLookupCacheV4.cpp:139: undefined reference toRefPtr<mozilla::safebrowsing::LookupCacheV4> SetupLookupCache<mozilla::safebrowsing::LookupCacheV4>(nsTArray<nsTString<char> > const&)'
[task 2020-02-20T14:48:17.350Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: ../../components/url-classifier/tests/gtest/Unified_cpp_tests_gtest0.o: in function UrlClassifierTableUpdateV4_EmptyUpdate2_Test::TestBody()': [task 2020-02-20T14:48:17.350Z] 14:48:17 INFO - /builds/worker/workspace/build/src/toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:763: undefined reference toRefPtr<mozilla::safebrowsing::LookupCacheV4> SetupLookupCache<mozilla::safebrowsing::LookupCacheV4>(nsTArray<nsTString<char> > const&)'
[task 2020-02-20T14:48:17.350Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: libxul.so: hidden symbol `_Z16SetupLookupCacheIN7mozilla12safebrowsing13LookupCacheV4EE6RefPtrIT_ERK8nsTArrayI9nsTStringIcEE' isn't defined
[task 2020-02-20T14:48:17.350Z] 14:48:17 INFO - /builds/worker/fetches/binutils/bin/ld: final link failed: bad value
[task 2020-02-20T14:48:17.351Z] 14:48:17 INFO - clang-9: error: linker command failed with exit code 1 (use -v to see invocation)
[task 2020-02-20T14:48:17.351Z] 14:48:17 INFO - /builds/worker/workspace/build/src/config/rules.mk:608: recipe for target 'libxul.so' failed
[task 2020-02-20T14:48:17.351Z] 14:48:17 ERROR - make[4]: *** [libxul.so] Error 1
[task 2020-02-20T14:48:17.351Z] 14:48:17 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/gtest'
[task 2020-02-20T14:48:17.351Z] 14:48:17 INFO - /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'toolkit/library/gtest/target' failed
[task 2020-02-20T14:48:17.351Z] 14:48:17 ERROR - make[3]: *** [toolkit/library/gtest/target] Error 2
[task 2020-02-20T14:48:17.351Z] 14:48:17 INFO - make[3]: *** Waiting for unfinished jobs....

I see, my bad. Apparently I did not run all relevant tests. I'll give it another shot.

Thanks Sylvestre.

Philipp, any progress on this? thanks!

Flags: needinfo?(zech.ph)

I've planned to look into it over the week-end. Busy week so far.

Flags: needinfo?(zech.ph)

First realization: forgot to run mach gtest which would have shown the error earlier.
Now trying to figure out how to change the tests when removing this function as it triggers quite some errors/necessary changes.

Attached file log.txt —

It seems that nBuildCache(LookupCacheV2* cache, const _PrefixArray& aPrefixArray) is still used after all. As already mentioned, running mach gtest reveals the usages (when removing both BuildCache(LookupCacheV2* cache, const _PrefixArray& aPrefixArray and RefPtr<T> SetupLookupCache(const _PrefixArray& aPrefixArray which depends on the former). The new, attached log shows the errors I now get. Does this still mean that the function is unused (BuildCache(LookupCacheV2* cache, const _PrefixArray& aPrefixArray) and I just continue fixing all the issues that pop up? Not that I'm not willing, I'm just not sure how to proceed exactly.

Flags: needinfo?(sledru)

BuildCache(LookupCacheV2* cache, const _PrefixArray& aPrefixArray) is still used by code here.

I guess the warning occurs because it is called in the function template SetupLookupCache(const _PrefixArray& aPrefixArray) and clang can't detect it somehow?

Seems to be the case, as this function is the first one to throw an error right after removing BuildCache(LookupCacheV2* cache, const _PrefixArray& aPrefixArray).

Any news here?

Really sorry about the latency. :(

You are right, the tool isn't probably looking at test.
We could probably update the test https://searchfox.org/mozilla-central/rev/a69f60390921e21e1d62d40ce388c44a71db00c6/toolkit/components/url-classifier/tests/gtest/TestCaching.cpp#237
as the function isn't used.
Or mark the bug as WONTFIX.

Sorry again for the latency and opening a non trivial bug after all (I have easier if you want)

Flags: needinfo?(sledru)

No problem!

I'm open to both options, yet, I have no opinion, as I honestly know to little about the whole software stack. If you decide to update the test and think I can do it/help in that, just let me know, I'd be happy to. Otherwise, if you want to close it as WONTFIX, it's also fine.

Philipp, thanks for your help!
I'll set this bug as INVALID because the function is actually used by another function (See Comment 20).

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: