New issue
Advanced search Search tips

Issue 759349 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 759881



Sign in to add a comment

Import google/crc32c as //third_party/crc32c

Project Member Reported by pwnall@chromium.org, Aug 27 2017

Issue description

Chrome currently uses the CRC32C implementation in LevelDB. We should replace it with https://github.com/google/crc32c, which was recently open-sourced. The code is available under the same license as LevelDB. We should import the library as //third_party/crc32c, and update all CRC32C usage to point to the new library.

Design doc: https://docs.google.com/document/d/1nNrYD3dUvqmbsYWooeVMu9oWRsm4wZdvS-R4ev1uFA0/
 

Comment 1 by pwnall@chromium.org, Aug 27 2017

Components: Blink>Storage
Labels: OS-All
Status: Assigned (was: Untriaged)

Comment 2 by pwnall@chromium.org, Aug 28 2017

Blockedon: 759881

Comment 3 by pwnall@chromium.org, Aug 29 2017

Description: Show this description
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc345b4acf606f9bb1cb214a36d3007d7c140947

commit dc345b4acf606f9bb1cb214a36d3007d7c140947
Author: Victor Costan <pwnall@chromium.org>
Date: Thu Aug 31 01:22:45 2017

Use google/crc32c instead of LevelDB's implementation.

Bug:  759349 
Change-Id: Ic2383645c302906bef7b8e2f5c39a0bd16c57e2f
Reviewed-on: https://chromium-review.googlesource.com/636008
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498698}
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/DEPS
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/components/sync/BUILD.gn
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/components/sync/engine/attachments/DEPS
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/components/sync/engine/attachments/attachment_util.cc
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/components/sync/engine_impl/attachments/DEPS
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/components/sync/engine_impl/attachments/attachment_downloader_impl_unittest.cc
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/third_party/.gitignore
[add] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/third_party/crc32c/BUILD.gn
[add] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/third_party/crc32c/OWNERS
[add] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/third_party/crc32c/README.chromium
[add] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/third_party/crc32c/config/crc32c/crc32c_config.h
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/third_party/leveldatabase/BUILD.gn
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/third_party/leveldatabase/README.chromium
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/third_party/leveldatabase/port/port_chromium.cc
[modify] https://crrev.com/dc345b4acf606f9bb1cb214a36d3007d7c140947/third_party/leveldatabase/port/port_chromium.h

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/041ac564c6952a712ed66fba41517fac29decc47

commit 041ac564c6952a712ed66fba41517fac29decc47
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Thu Aug 31 01:53:50 2017

Revert "Use google/crc32c instead of LevelDB's implementation."

This reverts commit dc345b4acf606f9bb1cb214a36d3007d7c140947.

Reason for revert: leading to a compile failure.
https://build.chromium.org/p/chromium.win/builders/Win%20x64%20Builder%20%28dbg%29/builds/57719

Original change's description:
> Use google/crc32c instead of LevelDB's implementation.
> 
> Bug:  759349 
> Change-Id: Ic2383645c302906bef7b8e2f5c39a0bd16c57e2f
> Reviewed-on: https://chromium-review.googlesource.com/636008
> Reviewed-by: Brett Wilson <brettw@chromium.org>
> Reviewed-by: Chris Mumford <cmumford@chromium.org>
> Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
> Reviewed-by: Chris Palmer <palmer@chromium.org>
> Commit-Queue: Victor Costan <pwnall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#498698}

TBR=palmer@chromium.org,brettw@chromium.org,cmumford@chromium.org,pavely@chromium.org,pwnall@chromium.org

Change-Id: Ic4eaf5027e3cd7d7fd938cbe1f2af9a445ebb1d9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  759349 
Reviewed-on: https://chromium-review.googlesource.com/644291
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498717}
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/DEPS
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/components/sync/BUILD.gn
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/components/sync/engine/attachments/DEPS
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/components/sync/engine/attachments/attachment_util.cc
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/components/sync/engine_impl/attachments/DEPS
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/components/sync/engine_impl/attachments/attachment_downloader_impl_unittest.cc
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/third_party/.gitignore
[delete] https://crrev.com/d6f5cd725ddf5ebc6ec23b3976bc367aa2b08a09/third_party/crc32c/BUILD.gn
[delete] https://crrev.com/d6f5cd725ddf5ebc6ec23b3976bc367aa2b08a09/third_party/crc32c/OWNERS
[delete] https://crrev.com/d6f5cd725ddf5ebc6ec23b3976bc367aa2b08a09/third_party/crc32c/README.chromium
[delete] https://crrev.com/d6f5cd725ddf5ebc6ec23b3976bc367aa2b08a09/third_party/crc32c/config/crc32c/crc32c_config.h
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/third_party/leveldatabase/BUILD.gn
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/third_party/leveldatabase/README.chromium
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/third_party/leveldatabase/port/port_chromium.cc
[modify] https://crrev.com/041ac564c6952a712ed66fba41517fac29decc47/third_party/leveldatabase/port/port_chromium.h

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0

commit 5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0
Author: Victor Costan <pwnall@chromium.org>
Date: Thu Aug 31 22:53:45 2017

Reland "Use google/crc32c instead of LevelDB's implementation."

This is a reland of dc345b4acf606f9bb1cb214a36d3007d7c140947

The crc32c library has been updated to 1.0.1, which fixes the build
issues.

Original change's description:
> Use google/crc32c instead of LevelDB's implementation.
> 
> Bug:  759349 
> Change-Id: Ic2383645c302906bef7b8e2f5c39a0bd16c57e2f
> Reviewed-on: https://chromium-review.googlesource.com/636008
> Reviewed-by: Brett Wilson <brettw@chromium.org>
> Reviewed-by: Chris Mumford <cmumford@chromium.org>
> Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
> Reviewed-by: Chris Palmer <palmer@chromium.org>
> Commit-Queue: Victor Costan <pwnall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#498698}

Bug:  759349 
Change-Id: I12a2cbe680b5fb0a15108f9c29d0b58c8b80e4b8
Reviewed-on: https://chromium-review.googlesource.com/644427
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499056}
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/DEPS
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/components/sync/BUILD.gn
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/components/sync/engine/attachments/DEPS
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/components/sync/engine/attachments/attachment_util.cc
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/components/sync/engine_impl/attachments/DEPS
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/components/sync/engine_impl/attachments/attachment_downloader_impl_unittest.cc
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/third_party/.gitignore
[add] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/third_party/crc32c/BUILD.gn
[add] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/third_party/crc32c/OWNERS
[add] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/third_party/crc32c/README.chromium
[add] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/third_party/crc32c/config/crc32c/crc32c_config.h
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/third_party/leveldatabase/BUILD.gn
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/third_party/leveldatabase/README.chromium
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/third_party/leveldatabase/port/port_chromium.cc
[modify] https://crrev.com/5286b0c773b21f08c515e7cf8dd1a3a69b7c47b0/third_party/leveldatabase/port/port_chromium.h

Status: Fixed (was: Assigned)
This stuck \o/

Comment 8 by sl1pk...@gmail.com, Sep 8 2017

[5483/32939] /tmp/makepkg/chromium-dev/src/chromium-62.0.3202.9/third_party/llvm-build/Release+Asserts/bin/clang++  -MMD -MF obj/third_party/crc32c/crc32c/crc32c.o.d -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=\"310694-2\" -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DHAVE_MM_PREFETCH=1 -DHAVE_SSE42=1 -DHAVE_BUILTIN_PREFETCH=1 -DHAVE_STRONG_GETAUXVAL=1 -I../.. -Igen -I../../third_party/crc32c/config -I../../third_party/crc32c/src/include -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -pthread -fcolor-diagnostics -m64 -march=x86-64 -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-unused-lambda-capture -Wno-user-defined-warnings -Wno-enum-compare-switch -O2 -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer -g0 -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-auto-raw-pointer -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -std=gnu++14 -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -D_FORTIFY_SOURCE=2 -march=native -O2 -pipe -fstack-protector-strong  -Wno-unknown-warning-option -c ../../third_party/crc32c/src/src/crc32c.cc -o obj/third_party/crc32c/crc32c/crc32c.o
FAILED: obj/third_party/crc32c/crc32c/crc32c.o 
/tmp/makepkg/chromium-dev/src/chromium-62.0.3202.9/third_party/llvm-build/Release+Asserts/bin/clang++  -MMD -MF obj/third_party/crc32c/crc32c/crc32c.o.d -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=\"310694-2\" -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DHAVE_MM_PREFETCH=1 -DHAVE_SSE42=1 -DHAVE_BUILTIN_PREFETCH=1 -DHAVE_STRONG_GETAUXVAL=1 -I../.. -Igen -I../../third_party/crc32c/config -I../../third_party/crc32c/src/include -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -pthread -fcolor-diagnostics -m64 -march=x86-64 -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Wno-unused-lambda-capture -Wno-user-defined-warnings -Wno-enum-compare-switch -O2 -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer -g0 -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-auto-raw-pointer -Xclang -plugin-arg-find-bad-constructs -Xclang check-ipc -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -std=gnu++14 -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -D_FORTIFY_SOURCE=2 -march=native -O2 -pipe -fstack-protector-strong  -Wno-unknown-warning-option -c ../../third_party/crc32c/src/src/crc32c.cc -o obj/third_party/crc32c/crc32c/crc32c.o
In file included from ../../third_party/crc32c/src/src/crc32c.cc:5:
In file included from ../../third_party/crc32c/src/include/crc32c/crc32c.h:41:
In file included from /usr/lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/string_view:39:
/usr/lib64/gcc/x86_64-pc-linux-gnu/7.2.0/../../../../include/c++/7.2.0/bits/c++17_warning.h:32:2: error: This file requires compiler and library support for the ISO C++ 2017 standard. This support must be enabled with the -std=c++17 or -std=gnu++17 compiler options.
#error This file requires compiler and library support \
 ^
In file included from ../../third_party/crc32c/src/src/crc32c.cc:5:
../../third_party/crc32c/src/include/crc32c/crc32c.h:44:35: error: no type named 'string_view' in namespace 'std'
inline uint32_t Crc32c(const std::string_view& string_view) {
                             ~~~~~^
2 errors generated.

clang from tools/clang/scripts/update.py
#8: What compiler are you using?

The .h file has some guards around enabling the string_view code before C++17, at https://cs.chromium.org/chromium/src/third_party/crc32c/src/include/crc32c/crc32c.h?l=36:

#if defined(__has_include)
#if __has_include(<string_view>)
// Visual Studio provides a <string_view> header even in C++11 mode. When
// included, the header issues an #error. (C1189)
#if !defined(_MSC_VER) || __cplusplus >= 201703L

It seems like the guards are not working on your compiler, so learning what it is gives me a chance to look into fixing the problem.
the compiler is clang 312679 downloaded with tools/clang/scripts/update.py

https://chromium.googlesource.com/chromium/src/tools/clang/+/master/scripts/update.py

also tested with system clang 4.0.1-5

all in linux
tested again with clang 5.0.0. same issue
Oh, duh, that information was in the command-line. Sorry.

This seems to be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79433

Can you please put the information in comment 8 in a new bug and mention it here? I'll prepare a fix.

Comment 13 Deleted

Comment 14 Deleted

Comment 15 Deleted

done: 763650

PS: why is automagic delete when paste the crbug URL?
Thanks! I don't know your URL got deleted, but I got to the bug and I'll update it when the fix lands.

Sign in to add a comment