New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 732670 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

SublimeClang indexing broken with "tu is None..." by SK_FREETYPE_MINIMUM_RUNTIME_VERSION define

Project Member Reported by jamescook@chromium.org, Jun 13 2017

Issue description

Chrome ToT r478712, on Linux with target_os=chromeos

* Set up SublimeClang per https://chromium.googlesource.com/chromium/src/+/master/docs/linux_sublime_dev.md
* Open base/base_switches.cc
* Good: Everything is indexed, autocomplete is smart, syntax errors are highlighted
* Open ash/ash_switches.cc
* Bad: Nothing is indexed, syntax errors are not highlighted

Will compile file /w/chrome/src/ash/ash_switches.cc with the following options:
['-I/usr/local/google/home/jamescook/.config/sublime-text-3/Packages/SublimeClang/internals/clang/include', '-Wno-attributes', '-x', 'c++', '-std=c++11', '-DUSE_CLANG_COMPLETER', '-std=c++11', '-x', 'c++', '-I/w/chrome/src', '-Wno-unknown-warning-option', '-DASH_IMPLEMENTATION', '-DV8_DEPRECATION_WARNINGS', '-DDCHECK_ALWAYS_ON=1', '-DUSE_UDEV', '-DUSE_ASH=1', '-DUSE_AURA=1', '-DUSE_NSS_CERTS=1', '-DUSE_OZONE=1', '-DDISABLE_NACL', '-DFULL_SAFE_BROWSING', '-DSAFE_BROWSING_CSD', '-DSAFE_BROWSING_DB_LOCAL', '-DCHROMIUM_BUILD', '-DFIELDTRIAL_TESTING_ENABLED', '-DCR_CLANG_REVISION=303910-1', '-D_FILE_OFFSET_BITS=64', '-D_LARGEFILE_SOURCE', '-D_LARGEFILE64_SOURCE', '-D__STDC_CONSTANT_MACROS', '-D__STDC_FORMAT_MACROS', '-DCOMPONENT_BUILD', '-DOS_CHROMEOS', '-DNDEBUG', '-DNVALGRIND', '-DDYNAMIC_ANNOTATIONS_ENABLED=0', '-DGL_GLEXT_PROTOTYPES', '-DUSE_GLX', '-DUSE_EGL', '-DANGLE_ENABLE_RELEASE_ASSERTS', '-DTOOLKIT_VIEWS=1', '-DU_USING_ICU_NAMESPACE=0', '-DU_ENABLE_DYLOAD=0', '-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE', '-DUCHAR_TYPE=uint16_t', '-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS', '-DSK_HAS_PNG_LIBRARY', '-DSK_HAS_WEBP_LIBRARY', '-DSK_HAS_JPEG_LIBRARY', '-DSK_FREETYPE_MINIMUM_RUNTIME_VERSION=(((FREETYPE_MAJOR)', '*', '0x01000000)', '|', '((FREETYPE_MINOR)', '*', '0x00010000)', '|', '((FREETYPE_PATCH)', '*', '0x00000100))', '-DSKIA_DLL', '-DGR_GL_IGNORE_ES3_MSAA=0', '-DSK_SUPPORT_GPU=1', '-DGOOGLE_PROTOBUF_NO_RTTI', '-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER', '-DHAVE_PTHREAD', '-DPROTOBUF_USE_DLLS', '-DMESA_EGL_NO_X11_HEADERS', '-DBORINGSSL_SHARED_LIBRARY', '-DUSING_V8_SHARED', '-I/w/chrome/src', '-I/w/chrome/src/out/Default/gen', '-I/w/chrome/src/third_party/libwebp', '-I/w/chrome/src/third_party/khronos', '-I/w/chrome/src/gpu', '-I/w/chrome/src/third_party/ced/src', '-I/w/chrome/src/third_party/icu/source/common', '-I/w/chrome/src/third_party/icu/source/i18n', '-I/w/chrome/src/skia/config', '-I/w/chrome/src/skia/ext', '-I/w/chrome/src/third_party/skia/include/c', '-I/w/chrome/src/third_party/skia/include/config', '-I/w/chrome/src/third_party/skia/include/core', '-I/w/chrome/src/third_party/skia/include/effects', '-I/w/chrome/src/third_party/skia/include/encode', '-I/w/chrome/src/third_party/skia/include/images', '-I/w/chrome/src/third_party/skia/include/lazy', '-I/w/chrome/src/third_party/skia/include/pathops', '-I/w/chrome/src/third_party/skia/include/pdf', '-I/w/chrome/src/third_party/skia/include/pipe', '-I/w/chrome/src/third_party/skia/include/ports', '-I/w/chrome/src/third_party/skia/include/utils', '-I/w/chrome/src/third_party/skia/third_party/vulkan', '-I/w/chrome/src/third_party/skia/include/gpu', '-I/w/chrome/src/third_party/skia/src/gpu', '-I/w/chrome/src/third_party/skia/src/sksl', '-I/w/chrome/src/build/linux/debian_jessie_amd64-sysroot/usr/include/dbus-1.0', '-I/w/chrome/src/build/linux/debian_jessie_amd64-sysroot/usr/lib/x86_64-linux-gnu/dbus-1.0/include', '-I/w/chrome/src/third_party/protobuf/src', '-I/w/chrome/src/out/Default/gen/protoc_out', '-I/w/chrome/src/third_party/protobuf/src', '-I/w/chrome/src/third_party/mesa/src/include', '-I/w/chrome/src/third_party/libwebm/source', '-I/w/chrome/src/third_party/boringssl/src/include', '-I/w/chrome/src/build/linux/debian_jessie_amd64-sysroot/usr/include/nss', '-I/w/chrome/src/build/linux/debian_jessie_amd64-sysroot/usr/include/nspr', '-I/w/chrome/src/third_party/WebKit', '-I/w/chrome/src/out/Default/gen/third_party/WebKit', '-I/w/chrome/src/v8/include', '-I/w/chrome/src/out/Default/gen/v8/include', '-I/w/chrome/src/third_party/qcms/src', '-I/w/chrome/src/out/Default/gen', '-fno-strict-aliasing', '-Wno-builtin-macro-redefined', '-D__DATE__=', '-D__TIME__=', '-D__TIMESTAMP__=', '-funwind-tables', '-fPIC', '-fcolor-diagnostics', '-m64', '-march=x86-64', '-Wall', '-Werror', '-Wextra', '-Wno-missing-field-initializers', '-Wno-unused-parameter', '-Wno-shift-negative-value', '-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', '-O2', '-fno-ident', '-fdata-sections', '-ffunction-sections', '-fno-omit-frame-pointer', '--sysroot=/w/chrome/src/build/linux/debian_jessie_amd64-sysroot', '-fvisibility=hidden', '-Wheader-hygiene', '-Wstring-conversion', '-Wtautological-overlap-compare', '-std=gnu++11', '-fno-rtti', '-fno-exceptions', '-fvisibility-inlines-hidden']
tu is None...

Git bisect shows it started with this CL:

Author: Ben Wagner <bungeman@chromium.org>
Date:   Mon Jun 5 14:45:34 2017 -0400

    Set build flag when using own FreeType.
    
    Set the SK_FREETYPE_MINIMUM_RUNTIME_VERSION build flag when the
    use_system_freetype gn flag is false. Currently the build flag is only
    set when is_win or is_mac, but this needs to be set any time it is
    known that FreeType will be built into the resulting binary. In
    particular this is desired for Android and WebView, but also for
    desktop Linux.
    
    BUG= chromium:715429 
    
    Change-Id: Ie5e425b0b347caf51400c5522a42e8a88605c147
    Reviewed-on: https://chromium-review.googlesource.com/524185

It causes the list of defines to have an item that has spaces in it.

    defines += [ "SK_FREETYPE_MINIMUM_RUNTIME_VERSION=(((FREETYPE_MAJOR) * 0x01000000) | ((FREETYPE_MINOR) * 0x00010000) | ((FREETYPE_PATCH) * 0x00000100))" ]

which some part of the SublimeClang tooling turns into a python list, chunked by spaces:

'-DSK_FREETYPE_MINIMUM_RUNTIME_VERSION=(((FREETYPE_MAJOR)', '*', '0x01000000)', '|', '((FREETYPE_MINOR)', '*', '0x00010000)', '|', '((FREETYPE_PATCH)', '*', '0x00000100))'

and '*' isn't a valid clang flag, so indexing dies.

You'll only see the indexing failure on files that have skia as a dependency (so it'll show up for files in //ash and //chrome but not //base).

This define seems really weird -- not only does it have spaces, but it's doing math.

bungemen, is there some other way we can compute this define value? scottmg suggested generating a header.

 
Cc: michaeln@chromium.org
Also, that define used to be computed with "<<" but that apparently caused Visual Studio problems, resulting in this CL:

commit 32fda6433bcc281eb4794b8715cf44edb3dd1ce2
Author: michaeln <michaeln@chromium.org>
Date:   Mon Apr 3 16:05:50 2017 -0700

    Fix visual studio project files around SK_FREETYPE_MINIMUM_RUNTIME_VERSION
    
    Replaced use of bit shifting operators with
    multiplication because visual studio fails to open projects using
    the '<<' operator in preprocessor definitions.
    
    BUG= 700926 
    
    Review-Url: https://codereview.chromium.org/2794893003
    Cr-Commit-Position: refs/heads/master@{#461572}

It seems like this has been a bit of a persistent issue for IDE tooling.

Taking a quick look at

https://github.com/quarnster/SublimeClang/blob/6823e7f0904e60680ac9f898108e301582ec5505/internals/translationunitcache.py

print("Will compile file %s with the following options:\n%s" % (filename, opts))

it appears that the output of the sublimeclang_options_script is read in as

opts += shlex.split(bdecode(output[0]))

where bdecode / sencode are just internal utf-8 handlers. However, just running

python tools/sublime/ninja_options_script.py -d '~/src/depot_tools' third_party/skia/src/ports/SkFontHost_FreeType.cpp

shows that while the chromium.ycm_extra_conf.py parsed everything fine, ninja_options_script.py doesn't write out this information in the format expected by the plugin (it doesn't shell escape it). I think this can be fixed by use of 'print pipes.quote(flag)' (or shlex.quote if only python 3).

Note that '-DCR_CLANG_REVISION=303910-1' also should be escaped, it should actually be '-DCR_CLANG_REVISION="303910-1"' (it has quotes).
I've created https://chromium-review.googlesource.com/c/533453/ which should fix this, but I don't use sublime, so someone who does should review.
Thanks for the analysis and writing the patch. Your CL works for me in Sublime and fixes my problem.

Note that there is also no reason for this define to show up when compiling ash/ash_switches.cc, it should only be set internally when building Skia. I've created https://chromium-review.googlesource.com/c/533573/ for that.
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 13 2017

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

commit 5d08c248cd71d45eebb97859ba172a5c882f1a0c
Author: Ben Wagner <bungeman@chromium.org>
Date: Tue Jun 13 16:49:31 2017

sublime/ninja_options_script.py shell quote output.

The sublimeclang plugin expects its input to be parseable by shlex.
The current output of ninja_options_script.py it not escaped,
leading to incorrect parsing. This changes ninja_options_script.py
to escape its output for parsing by shlex.

BUG= chromium:732670 

Change-Id: Ib57a8c15a3cc78df83ee79fa8d92a04b57531572
Reviewed-on: https://chromium-review.googlesource.com/533453
Commit-Queue: Ben Wagner <bungeman@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479051}
[modify] https://crrev.com/5d08c248cd71d45eebb97859ba172a5c882f1a0c/tools/sublime/ninja_options_script.py

Status: Fixed (was: Assigned)
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 13 2017

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

commit 6d96b8285c1d57854c6679c6d3e9dbef1e683d1b
Author: Ben Wagner <bungeman@chromium.org>
Date: Tue Jun 13 22:29:25 2017

Make SK_FREETYPE_MINIMUM_RUNTIME_VERSION private.

The define SK_FREETYPE_MINIMUM_RUNTIME_VERSION is only used when
building Skia the library -- it does not affect any headers. As a
result, it should only be set when building Skia, it does not need
to be defined when building dependencies.

BUG= chromium:732670 

Change-Id: I35eb988fb9e8af395e9f5d8668d7c0b3a5a02db8
Reviewed-on: https://chromium-review.googlesource.com/533573
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Commit-Queue: Ben Wagner <bungeman@google.com>
Cr-Commit-Position: refs/heads/master@{#479178}
[modify] https://crrev.com/6d96b8285c1d57854c6679c6d3e9dbef1e683d1b/skia/BUILD.gn

Comment 9 by jahag...@amazon.com, Jul 14 2017

Could you please merge this in M60. We are seeing this issue in while building 60.0.3112.66 but not on Master.
I'm assuming the change you're looking to merge to M60 is the one from Comment 6, which is just the change to the sublimeclang project generator? (There is also the change in Comment 8, which is just a build clean up.)
Cc: -bunge...@chromium.org jamescook@chromium.org
Owner: bunge...@chromium.org
bungeman, can you sort out the merge issue here? I didn't actually do anything with this bug other than file it. :-)

We're pretty close to the stable cut. OTOH, if all we want is the sublime script, that seems safe to merge.

We merge stuff to stable to ship a product. Why would we merge a change to an editor integration?
Apologies for not following up promptly. I was referring to comment#8 commit. 

While building M60 branch, I was facing build issues and cherry-picking commit#8 change seemed to fix the problem.
The change in comment #8 should have been purely cosmetic in nature, it simply causes a particular define be passed to the compiler for fewer translation units. For most translation units having this define set was just a no-op; outside of Skia the text SK_FREETYPE_MINIMUM_RUNTIME_VERSION doesn't appear, so the define is never actually used outside Skia code.

I'm unfamiliar with what build may have had errors and don't know what the build errors actually were. However, the Chromium / Chrome build system seems to be fine with how things were in M60. As a result I can only assume that the failing build contains some kind of transformation which doesn't properly escape defines (as tools/sublime/ninja_options_script.py did before the change in comment #6).

At this point I think it would be pretty difficult to convince the merge team to approve merging a build change like comment #8. It doesn't fix an actual bug in Chromium itself (it doesn't cause changes to any code), you have a work-around, and there aren't enough details here to rule out that the issue isn't an external build process improperly escaping defines.

Sign in to add a comment