A fair amount of places code SQL instructions using a const char[] (note the lack of static) in a function. Results (including places that use "static"): https://cs.chromium.org/search/?q=const%5C+char%5C+.*sql.*%5C%5B%5C%5D%5C+%5C%3D+-file:src/third_party/&p=5&sq=package:chromium&type=cs Clang 6 figures out the "static" regardless of optimization level, whereas gcc 8.1 optimizes to a static string with -Os, but initializes a string on the stack with -O2. So, while Chrome does not pay the cost for these mistakes, other embedders who use gcc might end up with worse code. Background: https://groups.google.com/a/chromium.org/d/topic/cxx/RByzL_eEOvc/discussion https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/pF7nAmUQ2uw/wrNhX5bN8YkJ
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1d868358c9c6bd34bcd9a14cd0384dfe9e91b153 commit 1d868358c9c6bd34bcd9a14cd0384dfe9e91b153 Author: Victor Costan <pwnall@chromium.org> Date: Tue Jun 26 19:06:48 2018 sql: Use static const char[] in local variables. Bug: 855465 Change-Id: I5c87955168ede7f73ab551c364066bde5909fb21 Reviewed-on: https://chromium-review.googlesource.com/1111858 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#570481} [modify] https://crrev.com/1d868358c9c6bd34bcd9a14cd0384dfe9e91b153/sql/connection.cc [modify] https://crrev.com/1d868358c9c6bd34bcd9a14cd0384dfe9e91b153/sql/connection_unittest.cc [modify] https://crrev.com/1d868358c9c6bd34bcd9a14cd0384dfe9e91b153/sql/meta_table_unittest.cc [modify] https://crrev.com/1d868358c9c6bd34bcd9a14cd0384dfe9e91b153/sql/recovery.cc [modify] https://crrev.com/1d868358c9c6bd34bcd9a14cd0384dfe9e91b153/sql/recovery_unittest.cc [modify] https://crrev.com/1d868358c9c6bd34bcd9a14cd0384dfe9e91b153/sql/sqlite_features_unittest.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/16013baf87ab26951698e0cc989b0b9209f8ccbf commit 16013baf87ab26951698e0cc989b0b9209f8ccbf Author: Xunran Ding <xunran.ding@samsung.com> Date: Fri Jul 13 05:21:00 2018 Qualify const char[] sql queries with |static| Bug: 855465 Change-Id: Idfe526ba88498413a280e243653667b56d18f6ae Reviewed-on: https://chromium-review.googlesource.com/1130566 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: Xunran Ding <xunran.ding@samsung.com> Cr-Commit-Position: refs/heads/master@{#574842} [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/blacklist/opt_out_blacklist/sql/opt_out_store_sql.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/history/core/browser/thumbnail_database.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/history/core/browser/thumbnail_database_unittest.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/history/core/browser/top_sites_database.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/history/core/browser/top_sites_database_unittest.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/background/request_queue_store_sql.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/model/add_page_to_download_manager_task.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/model/cleanup_thumbnails_task.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/model/clear_digest_task.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/model/clear_storage_task.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/model/complete_offline_page_upgrade_task.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/model/mark_page_accessed_task.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/model/start_offline_page_upgrade_task.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/model/update_file_path_task.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/offline_page_metadata_store.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/offline_page_metadata_store_test_util.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/offline_page_metadata_store_unittest.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/offline_pages/core/prefetch/store/prefetch_store_schema_unittest.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/components/omnibox/browser/shortcuts_database_unittest.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/content/browser/appcache/appcache_database.cc [modify] https://crrev.com/16013baf87ab26951698e0cc989b0b9209f8ccbf/storage/browser/quota/quota_database.cc
Thank you for working on this and improving our codebase! I think you missed a couple of places: https://cs.chromium.org/chromium/src/sql/test/test_helpers.cc?q=const%5C+char%5C+.*sql.*%5C%5B%5C%5D%5C+%5C%3D+-file:src/third_party/&sq=package:chromium&dr=C&l=23 https://cs.chromium.org/chromium/src/components/offline_pages/core/prefetch/store/prefetch_store_schema.cc?q=const%5C+char%5C+.*sql.*%5C%5B%5C%5D%5C+%5C%3D+-file:src/third_party/&sq=package:chromium&l=122&dr=C Can you please fix these as well?
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/35b72bf255d6519506b7e732f9c74205d2ab452d commit 35b72bf255d6519506b7e732f9c74205d2ab452d Author: Xunran Ding <xunran.ding@samsung.com> Date: Fri Aug 10 06:54:19 2018 fixup! Qualify const char[] sql queries with |static| Bug: 855465 Change-Id: I1736e5d83494341e068e4aeffeea535d624f3dd4 Reviewed-on: https://chromium-review.googlesource.com/1168944 Reviewed-by: Victor Costan <pwnall@chromium.org> Reviewed-by: Cathy Li <chili@chromium.org> Commit-Queue: Xunran Ding <xunran.ding@samsung.com> Cr-Commit-Position: refs/heads/master@{#582080} [modify] https://crrev.com/35b72bf255d6519506b7e732f9c74205d2ab452d/components/offline_pages/core/prefetch/store/prefetch_store_schema.cc [modify] https://crrev.com/35b72bf255d6519506b7e732f9c74205d2ab452d/sql/test/test_helpers.cc
Thank you very much for your work here!
Comment 1 by bugdroid1@chromium.org
, Jun 26 2018