New issue
Advanced search Search tips

Issue 867393 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 29
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug

Blocking:
issue 866225
issue 869067



Sign in to add a comment

-Wreturn-stack-address got more aggressive

Project Member Reported by thakis@chromium.org, Jul 25

Issue description

https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.clang%2FToTWin%2F1903%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout

FAILED: obj/chrome/test/unit_tests/desktop_ios_promotion_util_unittest.obj 
../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe /nologo /showIncludes  -imsvc..\..\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\win_sdk\Include\10.0.17134.0\um -imsvc..\..\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\win_sdk\Include\10.0.17134.0\shared -imsvc..\..\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\win_sdk\Include\10.0.17134.0\winrt -imsvc..\..\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\win_sdk\Include\10.0.17134.0\ucrt -imsvc..\..\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\VC\Tools\MSVC\14.14.26428\include -imsvc..\..\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\VC\Tools\MSVC\14.14.26428\atlmfc\include -DV8_DEPRECATION_WARNINGS -DUSE_AURA=1 -DNO_TCMALLOC -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DOFFICIAL_BUILD -DGOOGLE_CHROME_BUILD "-DCR_CLANG_REVISION=\"337761\"" -D_HAS_NODISCARD -D_HAS_EXCEPTIONS=0 -D__STD_C -D_CRT_RAND_S -D_CRT_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_DEPRECATE -D_ATL_NO_OPENGL -D_WINDOWS -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DPSAPI_VERSION=1 -DWIN32 -D_SECURE_ATL -D_USING_V110_SDK71_ -DWINAPI_FAMILY=WINAPI_FAMILY_DESKTOP_APP -DWIN32_LEAN_AND_MEAN -DNOMINMAX -D_UNICODE -DUNICODE -DNTDDI_VERSION=0x0A000002 -D_WIN32_WINNT=0x0A00 -DWINVER=0x0A00 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DGTEST_API_= -DGTEST_HAS_POSIX_RE=0 -DGTEST_LANG_CXX11=1 -DGTEST_HAS_TR1_TUPLE=0 -DWEBP_EXTERN=extern -DUSE_EGL -DTOOLKIT_VIEWS=1 -DEXPAT_RELATIVE_PATH -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUCHAR_TYPE=wchar_t -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DV8_USE_EXTERNAL_STARTUP_DATA -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_HAS_PNG_LIBRARY -DSK_HAS_WEBP_LIBRARY -DSK_HAS_JPEG_LIBRARY -DSK_SUPPORT_GPU=1 "-DSK_GPU_WORKAROUNDS_HEADER=\"gpu/config/gpu_driver_bug_workaround_autogen.h\"" -DGR_GL_FUNCTION_TYPE=__stdcall -DLEVELDB_PLATFORM_CHROMIUM=1 -DDeleteFile=DeleteFileW -DOS_WIN -DWEBRTC_NON_STATIC_TRACE_EVENT_HANDLERS=0 -DGTEST_RELATIVE_PATH -DWEBRTC_CHROMIUM_BUILD -DWEBRTC_WIN -DABSL_ALLOCATOR_NOTHROW=1 -DNO_MAIN_THREAD_WRAPPING -DMESA_EGL_NO_X11_HEADERS "-DI18N_ADDRESS_VALIDATION_DATA_URL=\"https://chromium-i18n.appspot.com/ssl-aggregate-address/\"" -DUNIT_TEST -DWTF_USE_WEBAUDIO_FFMPEG=1 -DSUPPORT_WEBGL2_COMPUTE_CONTEXT=1 -DWTF_USE_DEFAULT_RENDER_THEME=1 -D__STD_C -D_CRT_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_DEPRECATE -DUSE_LIBJPEG_TURBO=1 -DI18N_PHONENUMBERS_USE_ICU_REGEXP=1 -DI18N_PHONENUMBERS_USE_ALTERNATE_FORMATS=1 -DI18N_PHONENUMBERS_NO_THREAD_SAFETY=1 -DXML_STATIC -DRLZ_NETWORK_IMPLEMENTATION_CHROME_NET -DHUNSPELL_STATIC -DHUNSPELL_CHROME_CLIENT -DUSE_HUNSPELL -I../.. -Igen -I../../third_party/googletest/custom -I../../third_party/googletest/src/googletest/include -I../../third_party/libwebp/src -I../../third_party/wtl/include -I../../third_party/libyuv/include -I../../third_party/khronos -I../../gpu -I../../third_party/webrtc_overrides -I../../testing/gtest/include -I../../third_party/libyuv/include -I../../third_party/usrsctp/usrsctplib -I../../third_party/webrtc -I../../third_party/ced/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/boringssl/src/include -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/encode -I../../third_party/skia/include/gpu -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl -I../../third_party/leveldatabase -I../../third_party/leveldatabase/src -I../../third_party/leveldatabase/src/include -I../../third_party/libwebm/source -I../../v8/include -Igen/v8/include -I../../third_party/webrtc_overrides -I../../third_party/webrtc -Igen/third_party/metrics_proto -I../../third_party/re2/src -I../../third_party/mesa/src/include -Igen -Igen -I../../third_party/libaddressinput/src/cpp/include -I../../third_party/perfetto/include -Igen/third_party/perfetto/protos -Igen/third_party/perfetto/protos -Igen/third_party/perfetto/protos -Igen -I../../third_party/googletest/custom -I../../third_party/googletest/src/googlemock/include -I../../third_party/cacheinvalidation/overrides -I../../third_party/cacheinvalidation/src -I../../third_party/flatbuffers/src/include -I../../third_party/blink/renderer/platform/wtf/os-win32 -I../../third_party/libjpeg_turbo -I../../third_party/iccjpeg -I../../third_party/libpng -I../../third_party/zlib -I../../third_party/ots/include -I../../v8/include -Igen/v8/include -I../../third_party/libphonenumber/dist/cpp/src -Igen/third_party/libphonenumber -I../../third_party/libxml/src/include -I../../third_party/libxml/win32/include -I../../third_party/webrtc_overrides -I../../testing/gtest/include -I../../third_party/webrtc -I../../third_party/expat/files/lib /utf-8 /X -fcolor-diagnostics -fmerge-all-constants -Xclang -mllvm -Xclang -instcombine-lower-dbg-declare=0 -no-canonical-prefixes -fcomplete-member-pointers /Gy /FS /bigobj /d2FastFail /Zc:sizedDealloc- -fmsc-version=1911 -m32 /Brepro /W4 -Wimplicit-fallthrough -Wthread-safety /WX /wd4091 /wd4127 /wd4251 /wd4275 /wd4312 /wd4324 /wd4351 /wd4355 /wd4503 /wd4589 /wd4611 /wd4100 /wd4121 /wd4244 /wd4505 /wd4510 /wd4512 /wd4610 /wd4838 /wd4995 /wd4996 /wd4456 /wd4457 /wd4458 /wd4459 /wd4200 /wd4201 /wd4204 /wd4221 /wd4018 /wd4245 /wd4267 /wd4305 /wd4389 /wd4702 /wd4701 /wd4703 /wd4661 /wd4706 /wd4715 /wd4267 -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-unneeded-internal-declaration -Wno-undefined-var-template -Wno-address-of-packed-member -Wno-nonportable-include-path -Wno-user-defined-warnings -Wno-unused-lambda-capture -Wno-null-pointer-arithmetic -Wno-enum-compare-switch -Wno-ignored-pragma-optimize -Wno-memset-transposed-args /O1 /Ob2 /Oy- /Zc:inline /Gw /Oi /MT -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare /wd4800 -Wno-inconsistent-missing-override -imsvc ../../third_party/abseil-cpp -Wno-inconsistent-missing-override -Wno-exit-time-destructors /wd4305 /wd4324 /wd4714 /wd4800 /wd4996 /wd4344 /wd4706 -DLIBXML_STATIC= /TP /wd4577 /GR- /c ../../chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc /Foobj/chrome/test/unit_tests/desktop_ios_promotion_util_unittest.obj /Fd"obj/chrome/test/unit_tests_cc.pdb"
../../chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc(98,12):  error: returning reference to local temporary object [-Werror,-Wreturn-stack-address]
    return identity_test_environment_.identity_manager()


...for some reason only on Windows though?

The warning looks correct: This returns an AccountInfo by value:

  AccountInfo GetPrimaryAccountInfo() const;

But this returns a reference to one of its fields:

  const std::string& account_id() {
    return identity_test_environment_.identity_manager()
        ->GetPrimaryAccountInfo()
        .account_id;
  }


The fix is probably just to change the return type to std::string there, but it'd be good to understand why this warning is now firing on the clang/win/tot bots but apparently nowhere else.
 
clang range: 337709:337761
------------------------------------------------------------------------
r337743 | rsmith | 2018-07-23 17:21:22 -0400 (Mon, 23 Jul 2018) | 2 lines

Fold -Wreturn-stack-address into general initialization lifetime
checking.
------------------------------------------------------------------------


That looks pretty related.


And it's win-only because we build this file only on win: https://cs.chromium.org/chromium/src/chrome/test/BUILD.gn?type=cs&q=desktop_ios_promotion_util_unittest&sq=package:chromium&g=0&l=4252
Owner: thakis@chromium.org
Status: Started (was: Untriaged)
CL to fix compile (https://chromium-review.googlesource.com/c/chromium/src/+/1150082) found that the tests currently depend on the test code being buggy. We're discussing on the CL what to do...
Blocking: 866225
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 26

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

commit 102860d12bd1cc62d30ce80c1a9a73c734199b69
Author: Nico Weber <thakis@chromium.org>
Date: Thu Jul 26 13:06:35 2018

Don't return a reference to a temporary.

Found by -Wreturn-stack-address with a new version of clang.

Fix test to still pass now that the test code works (thanks jochen@
for how to do this).

Bug:  867393 
Change-Id: Ib732ec845726ab5174ad612bdb564ae238115999
Reviewed-on: https://chromium-review.googlesource.com/1150082
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578275}
[modify] https://crrev.com/102860d12bd1cc62d30ce80c1a9a73c734199b69/chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util_unittest.cc

Status: Fixed (was: Started)
Blocking: 869067

Sign in to add a comment