rewrite_to_chrome_style: we need to ensure the tool runs fine on all platforms |
|||||||||
Issue descriptionStandard library on Linux uses some less common language constructs - in the past these constructs tripped Clang AST matchers library (preventing the tool from running altogether). We should try to run the renaming too on all platforms, to ensure we have time to hunt down and fix AST matcher issues (if any) that might only show up in presence of platform-specific headers. AFAIK, so far we've only run the renaming tool on Linux.
,
Jan 6 2017
Verified it runs on linux on an android build. But there are a couple assertions that it hits in clang.
Processed 8857 files with rewrite_to_chrome_style tool (0 failures) [37.04%]
Failed to process /usr/local/google/home/danakj/s/c/src/out_android/Release/../../sdch/open-vcdiff/src/blockhash.cc
warning: ../../third_party/llvm-build/Release+Asserts/bin/clang++: 'linker' input unused [-Wunused-command-line-argument]
rewrite_to_chrome_style: /usr/local/google/home/danakj/s/c/src/third_party/llvm/tools/clang/lib/ASTMatchers/ASTMatchFinder.cpp:671: bool clang::ast_matchers::internal::{anonymous}::MatchASTVisitor::matchesAncestorOfRecursively(const clang::ast_type_traits::DynTypedNode&, const clang::ast_matchers::internal::DynTypedMatcher&, clang::ast_matchers::internal::BoundNodesTreeBuilder*, clang::ast_matchers::internal::ASTMatchFinder::AncestorMatchMode): Assertion `!Parents.empty() && "Found node that is not in the parent map."' failed.
Processed 12071 files with rewrite_to_chrome_style tool (1 failures) [50.47%]
Failed to process /usr/local/google/home/danakj/s/c/src/out_android/Release/../../third_party/skia/src/core/SkLinearBitmapPipeline.cpp
warning: ../../third_party/llvm-build/Release+Asserts/bin/clang++: 'linker' input unused [-Wunused-command-line-argument]
rewrite_to_chrome_style: /usr/local/google/home/danakj/s/c/src/third_party/llvm/tools/clang/lib/ASTMatchers/ASTMatchFinder.cpp:671: bool clang::ast_matchers::internal::{anonymous}::MatchASTVisitor::matchesAncestorOfRecursively(const clang::ast_type_traits::DynTypedNode&, const clang::ast_matchers::internal::DynTypedMatcher&, clang::ast_matchers::internal::BoundNodesTreeBuilder*, clang::ast_matchers::internal::ASTMatchFinder::AncestorMatchMode): Assertion `!Parents.empty() && "Found node that is not in the parent map."' failed.
Neither of these files has any replacements of course, and if I run the tool on the files instead of --all, it does not assert.
,
Jan 31 2017
The assert from #c2 happens for quite a few Blink sources when running the clang tool on Windows. Example: Failed to process D:/src/chromium/src/out/rel\../../third_party/WebKit/Source/web/WebNodeTest.cpp Assertion failed: !Parents.empty() && "Found node that is not in the parent map.", file D:\src\chromium\src\third_party\llvm\tools\clang\lib\ASTMatchers\ASTMatchFinder.cpp, line 671
,
Jan 31 2017
#c3 repros with a source file containing only a single line: #include <stddef.h>.
I have trouble narrowing it down further, because pasting the contents of stddef.h into into a repro-source-file leads to trouble including <__config> and/or with the compiler complaining that #pragma system_header cannot be used in the main source file.
I am also not quite sure which of the many stddef.h files ends up being used in my repro.
PS D:\> dir -Path D:/src/chromium/src -Recurse -ErrorAction SilentlyContinue -Filter stddef.h | ?{ -not $_.PSIsContainer } | Measure-Object | % Count
17
,
Feb 1 2017
Dumping the node that triggers the assert gives: ParmVarDecl 0x2316399d608 <D:\src\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\win_sdk\bin\..\..\VC\include\iosfwd:160:24, col:31> col:31 used _First1 'char16_t *'
,
Feb 2 2017
+brucedawson@ +thakis@ [possibly as a proxy for adding more people working on clang / Windows intersection] I need some advise on the following: The assert from #c5 that points its finger at <iosfwd> happens even if I strip the source file from *any* includes - this is rather surprising and makes narrowing the repro a bit difficult (can't get much more narrow than an empty file...). Q1. Is there a set of std library headers that are included even if they are not mentioned in the body of the source file being compiled? Q2. Is there a command line flag that I might be able to pass to clang to avoid this behavior? BTW: I did verify that the source file I am modifying above (the "empty file") is impacting the repro (i.e. that I am modifying the "right" file) - if I insert some garbage into the file, then in addition to the assert from #c5 I will also see a syntax error reported. I've tried to work around the issue above, by directly editing D:\src\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\win_sdk\bin\..\..\VC\include\iosfwd, but that meant that the compiler would complain that the pregenerated pch headers are stale and wouldn't go much further (unless I run ninja on the whole build again which takes considerable time...). Because of the overhead of rebuilding the pregeneratd pch headers via full ninja build this doesn't look promising, but please let me know if you think I should pursue this further (rather than focusing on Q1 and Q2 above). BTW: This is what the offending line in iosfwd looks like below (as you can see I've tried commenting out the SAL annotation, but apparently this didn't get rid of the assert - the assert is probably caused by something else like specific usage pattern of templates [similarily to https://llvm.org/bugs/show_bug.cgi?id=29042]): ParmVarDecl 0x21158d99908 <D:\src\depot_tools\win_toolchain\vs_files\d3cb0e37bdd120ad0ac4650b674b09e81be45616\win_sdk\bin\..\..\VC\include\iosfwd:160:30, col:37> col:37 used _First1 'char16_t *' ... template<class _Elem, class _Int_type> struct _Char_traits { ... static _Elem *__CLRCALL_OR_CDECL copy( line 160 -> /* _Out_writes_(_Count) */ _Elem *_First1, <- line 160 is HERE _In_reads_(_Count) const _Elem *_First2, size_t _Count) { // copy [_First2, _First2 + _Count) to [_First1, ...) _Elem *_Next = _First1; for (; 0 < _Count; --_Count, ++_Next, ++_First2) assign(*_Next, *_First2); return (_First1); } Hmmm... maybe I should try to take the above and see if it will tickle the assert on a linux build of clang.
,
Feb 2 2017
In case it helps, I've copy&pasted the related command from the compile_commands.json database used by rewrite_to_chrome_style.exe tool (I've been editing ParsedContentType.cpp [up to making it an empty file] when trying to narrow down the repro):
"command": "../../third_party/llvm-build/Release+Asserts/bin/clang-cl.exe --driver-mode=cl /nologo /showIncludes /FC \"-imsvcD:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/win_sdk/bin/../../win_sdk/Include/10.0.14393.0/um\" \"-imsvcD:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/win_sdk/bin/../../win_sdk/Include/10.0.14393.0/shared\" \"-imsvcD:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/win_sdk/bin/../../win_sdk/Include/10.0.14393.0/winrt\" \"-imsvcD:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/win_sdk/bin/../../win_sdk/Include/10.0.14393.0/ucrt\" \"-imsvcD:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/win_sdk/bin/../../VC/include\" \"-imsvcD:/src/depot_tools/win_toolchain/vs_files/d3cb0e37bdd120ad0ac4650b674b09e81be45616/win_sdk/bin/../../VC/atlmfc/include\" -DBLINK_PLATFORM_IMPLEMENTATION=1 -DINSIDE_BLINK -DV8_DEPRECATION_WARNINGS -DUSE_AURA=1 -DNO_TCMALLOC -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=289944-2 -D__STD_C -D_CRT_RAND_S -D_CRT_SECURE_NO_DEPRECATE -D_HAS_EXCEPTIONS=0 -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_ -DWIN32_LEAN_AND_MEAN -DNOMINMAX -D_UNICODE -DUNICODE -DNTDDI_VERSION=0x0A000000 -D_WIN32_WINNT=0x0A00 -DWINVER=0x0A00 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DUSE_EGL -DSK_IGNORE_DW_GRAY_FIX -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_SUPPORT_GPU=1 -DGR_GL_FUNCTION_TYPE=__stdcall -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -D__STD_C -D_CRT_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_DEPRECATE -DENABLE_LAYOUT_UNIT_IN_INLINE_BOXES=0 -DENABLE_OILPAN=1 -DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1 -DWTF_USE_WEBAUDIO_FFMPEG=1 -DWTF_USE_DEFAULT_RENDER_THEME=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUSE_LIBJPEG_TURBO=1 -DENABLE_IPC_FUZZER -Igen/blink -I../../third_party/ffmpeg -I../.. -Igen -I../../third_party/WebKit/Source -I../../third_party/WebKit -Igen/blink -Igen/third_party/WebKit -I../../third_party/khronos -I../../gpu -I../../third_party/wtl/include -I../../third_party/libwebp -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/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/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl -I../../third_party/libwebm/source -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/boringssl/src/include -I../../third_party/WebKit/Source/wtf/os-win32 -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -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/ced/src -I../../third_party/harfbuzz-ng/src -I../../third_party/ffmpeg/chromium/config/Chromium/win/x64 -I../../third_party/ffmpeg /wd4334 /wd4724 -fcolor-diagnostics -fdiagnostics-absolute-paths /Gy /FS /bigobj /d2FastFail /Zc:sizedDealloc- -fmsc-version=1900 -m64 /W4 /WX /utf-8 /wd4091 /wd4127 /wd4251 /wd4351 /wd4355 /wd4503 /wd4589 /wd4611 /wd4100 /wd4121 /wd4244 /wd4505 /wd4510 /wd4512 /wd4610 /wd4838 /wd4995 /wd4996 /wd4456 /wd4457 /wd4458 /wd4459 /wd4312 -Wno-microsoft-enum-value -Wno-unknown-pragmas -Wno-microsoft-cast -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member /O1 /Ob2 /Oy- /d2Zi+ /Zc:inline /Gw /Oi /MT -Xclang -add-plugin -Xclang find-bad-constructs -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare /wd4267 /FI../../third_party/WebKit/Source/platform/win/Precompile-platform.h /wd4305 /wd4324 /wd4714 /wd4800 /wd4996 -Xclang -add-plugin -Xclang blink-gc-plugin -Wglobal-constructors /wd4344 /wd4706 /Fpobj/third_party/WebKit/Source/platform/platform_cc.pch /Yu../../third_party/WebKit/Source/platform/win/Precompile-platform.h /TP /GR- /c ../../third_party/WebKit/Source/platform/network/ParsedContentType.cpp /Foobj/third_party/WebKit/Source/platform/platform/ParsedContentType.obj /Fdobj/third_party/WebKit/Source/platform/platform_cc.pdb",
,
Feb 2 2017
#c7 helped me diagnose this myself - I can no longer repro after removing the following parts of the command line: /FI../../third_party/WebKit/Source/platform/win/Precompile-platform.h /Fpobj/third_party/WebKit/Source/platform/platform_cc.pch /Yu../../third_party/WebKit/Source/platform/win/Precompile-platform.h This is enough for unblocking my efforts to narrow down further the AST matcher assert. Sorry for the noise :-)
,
Feb 2 2017
> I am also not quite sure which of the many stddef.h > files ends up being used in my repro. /showIncludes will show you full path names to all files that are included Also, /P (preprocess file to a .i file) can be useful for creating standalone reproes. You should usually be able to rename the .i file to .cc and then compile it. The preprocessed file will contain #file markers for which file is responsible for various bits of code.
,
Feb 2 2017
,
Feb 2 2017
,
Feb 3 2017
,
Feb 3 2017
,
Jul 21 2017
,
Jul 21 2017
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by lukasza@chromium.org
, Dec 28 2016Status: Assigned (was: Untriaged)