Bug 207119 – Enable offlineasm debug annotations for GCC

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Angelos Oikonomopoulos]

Enable offlineasm debug annotations for GCC

[Angelos Oikonomopoulos]

Created attachment 389516 [details]
Patch

[Angelos Oikonomopoulos]

r?

[Angelos Oikonomopoulos]

ping

[Angelos Oikonomopoulos]

Created attachment 394270 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 394270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394270&action=review

> Source/JavaScriptCore/CMakeLists.txt:141
> +option(GCC_OFFLINEASM_SOURCE_MAP
> +  "Produce debug line information for offlineasm-generated code")
> +
> +
> +# Enable by default for Debug builds, allow the user to manually
> +# request source mapping for Release builds.
> +if (CMAKE_BUILD_TYPE STREQUAL "Debug")
> +  set(GCC_OFFLINEASM_SOURCE_MAP "ON")
> +endif ()

I wonder if we should enable this also by default also on RelWithDebugInfo builds? How much more extra megabytes causes it to grow the debug info on a release build if enabled?

The way you override the value in case of "Debug" build makes it impossible to manually disable it (on Debug builds)
Instead of doing that, I suggest to set a default value variable and use that variable as the default value for the cmake option.
Example:

set(GCC_OFFLINEASM_SOURCE_MAP_DEFAULT OFF)
if (CMAKE_BUILD_TYPE STREQUAL "Debug" OR CMAKE_BUILD_TYPE STREQUAL "RelWithDebInfo")
  set(GCC_OFFLINEASM_SOURCE_MAP_DEFAULT ON)
endif()

option(GCC_OFFLINEASM_SOURCE_MAP
  "Produce debug line information for offlineasm-generated code"
  ${GCC_OFFLINEASM_SOURCE_MAP_DEFAULT})

[Konstantin Tokarev]

Comment on attachment 394270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394270&action=review

>> Source/JavaScriptCore/CMakeLists.txt:141
>> +endif ()
> 
> I wonder if we should enable this also by default also on RelWithDebugInfo builds? How much more extra megabytes causes it to grow the debug info on a release build if enabled?
> 
> The way you override the value in case of "Debug" build makes it impossible to manually disable it (on Debug builds)
> Instead of doing that, I suggest to set a default value variable and use that variable as the default value for the cmake option.
> Example:
> 
> set(GCC_OFFLINEASM_SOURCE_MAP_DEFAULT OFF)
> if (CMAKE_BUILD_TYPE STREQUAL "Debug" OR CMAKE_BUILD_TYPE STREQUAL "RelWithDebInfo")
>   set(GCC_OFFLINEASM_SOURCE_MAP_DEFAULT ON)
> endif()
> 
> option(GCC_OFFLINEASM_SOURCE_MAP
>   "Produce debug line information for offlineasm-generated code"
>   ${GCC_OFFLINEASM_SOURCE_MAP_DEFAULT})

It should take much less than other kinds of debug info. I also think it should be moved to OptionsCommon.cmake near to DEBUG_FISSION (-gsplit-dwarf) code. AFAIK we have all option()s under Source/cmake and it would be surprising to find another one elsewhere.

BTW, can't it be worked around by forcing gcc to call llvm-as?

[Angelos Oikonomopoulos]

Yah, enabling it for RelWithDebInfo is a good idea. The size increase is minimal, on X86_64 libJavaScriptCore.so goes from 796100296B to 796196048B, for an increase of 0.012%.

I don't expect one would need to disable this during normal development (e.g. can't do that under clang either) but the extra flexibility can't hurt!

OptionsCommon.cmake seems like a more appropriate location for the option, yah.

> BTW, can't it be worked around by forcing gcc to call llvm-as?

AFAICT you can only set the the assembler GCC uses at configure time (such an option would make things easier for the approach in this patch too, as we wouldn't need to wrap the C++ compiler).

What's more, llvm-as may not be available in the build environment or it might not be straightforward to configure GCC to use it (one example might be when using a cross-compiling toolchain from a vendor).

Will try to update the patch with both your suggestions.

[Angelos Oikonomopoulos]

Created attachment 394363 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 394363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394363&action=review

> Source/JavaScriptCore/CMakeLists.txt:149
> +  set(CMAKE_CXX_COMPILER_LAUNCHER "${JavaScriptCore_SCRIPTS_SOURCES_DIR}/cxx-wrapper")

This will conflict with ccache. Maybe append to ${CMAKE_CXX_COMPILER_LAUNCHER} ?

[Konstantin Tokarev]

Comment on attachment 394363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394363&action=review

> Source/WTF/wtf/Compiler.h:391
> +#define MARKER_SYMBOL(name) void NO_REORDER name(void) { __asm__(""); }

Shouldn't it be "asm volatile" (or "__asm__ __volatile")?

[Konstantin Tokarev]

Comment on attachment 394363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394363&action=review

> Source/JavaScriptCore/CMakeLists.txt:156
> +    PROPERTIES

We use 4 spaces for indentation

> Source/JavaScriptCore/Scripts/cxx-wrapper:1
> +#!/bin/sh

Script is missing license header

> Source/JavaScriptCore/Scripts/cxx-wrapper:12
> +	postprocess=1

We don't use tabs for indentation, when possible

> Source/JavaScriptCore/Scripts/postprocess-asm:1
> +#!/usr/bin/env ruby

Script is missing license header. Also I heard there were problems with /usr/bin/env shebangs in some distros

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:540
> +#if defined(POSTPROCESS_ASM) && COMPILER(GCC)

It should not be GCC-only, as binaries produced with Clang also need fix to be debuggable with gdb

>> Source/WTF/wtf/Compiler.h:391
>> +#define MARKER_SYMBOL(name) void NO_REORDER name(void) { __asm__(""); }
> 
> Shouldn't it be "asm volatile" (or "__asm__ __volatile")?

Nvm, I've overloooked that it's a complete function definition here.

[Carlos Alberto Lopez Perez]

Comment on attachment 394363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394363&action=review

>> Source/JavaScriptCore/CMakeLists.txt:149
>> +  set(CMAKE_CXX_COMPILER_LAUNCHER "${JavaScriptCore_SCRIPTS_SOURCES_DIR}/cxx-wrapper")
> 
> This will conflict with ccache. Maybe append to ${CMAKE_CXX_COMPILER_LAUNCHER} ?

Good point.
But i think it should prepend (not append). The shell script wrapper should execute before than ccache.
Otherwise ccache will think the shell script its the compiler and would cache the hash of the shell script instead of hashing the hash of the real compiler.

>> Source/JavaScriptCore/Scripts/postprocess-asm:1
>> +#!/usr/bin/env ruby
> 
> Script is missing license header. Also I heard there were problems with /usr/bin/env shebangs in some distros

That its a guideline fedora has for shipping executable scripts in fedora packages.
This is a build script, not part of what gets installed, so its ok.
See bug 208970

[Yusuke Suzuki]

How about using wrapped-GCC command only for LowLevelInterpreter.cpp and ensure LLIntAssembly.h is only included by this file?

[Konstantin Tokarev]

(In reply to Yusuke Suzuki from comment #13)
> How about using wrapped-GCC command only for LowLevelInterpreter.cpp and
> ensure LLIntAssembly.h is only included by this file?

That's not trivial to do with cmake, though it should be possible to do if LowLevelInterpreter.cpp is segregated into its own build target (e.g. STATIC or OBJECT library). In this case it's possible to set CXX_COMPILER_LAUNCHER property on that target specifically, without affecting compilation of other files. It should also resolve concerns about ccache, as not caching compilation of one file is acceptable.

[Konstantin Tokarev]

Comment on attachment 394363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394363&action=review

>>> Source/JavaScriptCore/CMakeLists.txt:149
>>> +  set(CMAKE_CXX_COMPILER_LAUNCHER "${JavaScriptCore_SCRIPTS_SOURCES_DIR}/cxx-wrapper")
>> 
>> This will conflict with ccache. Maybe append to ${CMAKE_CXX_COMPILER_LAUNCHER} ?
> 
> Good point.
> But i think it should prepend (not append). The shell script wrapper should execute before than ccache.
> Otherwise ccache will think the shell script its the compiler and would cache the hash of the shell script instead of hashing the hash of the real compiler.

Indeed. While actual compiler doesn't produce .o file here, ccache should still be able to cache generated assembly file.

>>> Source/JavaScriptCore/Scripts/postprocess-asm:1
>>> +#!/usr/bin/env ruby
>> 
>> Script is missing license header. Also I heard there were problems with /usr/bin/env shebangs in some distros
> 
> That its a guideline fedora has for shipping executable scripts in fedora packages.
> This is a build script, not part of what gets installed, so its ok.
> See bug 208970

Is generate generate-gtkdoc installed? I thought it was also build-only.

[Carlos Alberto Lopez Perez]

(In reply to Konstantin Tokarev from comment #15)
> >>> Source/JavaScriptCore/Scripts/postprocess-asm:1
> >>> +#!/usr/bin/env ruby
> >> 
> >> Script is missing license header. Also I heard there were problems with /usr/bin/env shebangs in some distros
> > 
> > That its a guideline fedora has for shipping executable scripts in fedora packages.
> > This is a build script, not part of what gets installed, so its ok.
> > See bug 208970
> 
> Is generate generate-gtkdoc installed? I thought it was also build-only.

It is build-only. Check: https://bugs.webkit.org/show_bug.cgi?id=208970#c8

[Angelos Oikonomopoulos]

Created attachment 394511 [details]
Patch

[Angelos Oikonomopoulos]

Thanks for the comments everyone!

Updated patch with the following changes:
- add a separate target for LowLevelInterpreter.cpp (so we only need to wrap one compiler invocation). Drop cxx-wrapper.
- prepend postprocess-asm to the target's RULE_LAUNCH_COMPILE property, so that we respect the ccache setting.
- add licence headers to the two remaining scripts.
- untabify CMakeLists.txt.
- unconditionally emit marker symbols around the top-level inline asm statement in LowLevelInterpreter.cpp, as this has nothing to do with the build toolchain and everything to do with the ability to debug using gdb. However, clang doesn't currently support __no_reorder__ so this is effectively a no-op.

[Angelos Oikonomopoulos]

Created attachment 394515 [details]
Patch

[Angelos Oikonomopoulos]

(In reply to Angelos Oikonomopoulos from comment #18)

> However, clang doesn't currently support __no_reorder__ so this is
> effectively a no-op.

So this needs to be gated on COMPILER(GCC) still in LowLevelInterpreter.cpp. Patch updated.

[Konstantin Tokarev]

(In reply to Angelos Oikonomopoulos from comment #18)
> - unconditionally emit marker symbols around the top-level inline asm
> statement in LowLevelInterpreter.cpp, as this has nothing to do with the
> build toolchain and everything to do with the ability to debug using gdb.
> However, clang doesn't currently support __no_reorder__ so this is
> effectively a no-op.

Do I understand correctly that GDB just needs to have .cfi_startproc and .cfi_endproc here to start using following .file and .loc directives?

If so, we might want to emit minimal assembly stub here via asm volatile instead of inserting unused function body. I think it may express intention more clearly even for GCC case, and should work with Clang as well.

[Konstantin Tokarev]

Comment on attachment 394515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394515&action=review

> Source/WTF/wtf/Compiler.h:391
> +#define MARKER_SYMBOL(name) void NO_REORDER name(void) { __asm__(""); }

Wouldn't it be enough to define NO_REORDER as empty for non-gcc? Yes clang will be able to move definition of marker, but maybe gdb can be satisfied by having entry point somewhere, not in exact location?

[Konstantin Tokarev]

Actually, it seems to me that cleaner solution would be to emit cfi directives in offlineasm instead of relying on MARKER_SYMBOL hacks

[Angelos Oikonomopoulos]

(In reply to Konstantin Tokarev from comment #21)
> (In reply to Angelos Oikonomopoulos from comment #18)
> > - unconditionally emit marker symbols around the top-level inline asm
> > statement in LowLevelInterpreter.cpp, as this has nothing to do with the
> > build toolchain and everything to do with the ability to debug using gdb.
> > However, clang doesn't currently support __no_reorder__ so this is
> > effectively a no-op.
> 
> Do I understand correctly that GDB just needs to have .cfi_startproc and
> .cfi_endproc here to start using following .file and .loc directives?

As far as I understand, that's not the issue. The issue is that no DW_AT_{low,high}_pc (or even DW_AT_ranges) is generated for LowLevelInterpreter.cpp. Then, dwarf2_record_block_ranges won't record an address range for it (https://github.com/bminor/binutils-gdb/blob/master/gdb/dwarf2/read.c#L14021) and find_pc_sect_compunit_symtab will not be able to resolve any PC to this CU (https://github.com/bminor/binutils-gdb/blob/19a2740f7f2ea0f65745a3c00cf8a64647378aa3/gdb/symtab.c#L2911).

Not sure why the dwarf attributes are not generated. Testing on a simpler testcase, I can confirm that
- .cfi_{start,end}proc don't make a difference
- .type foo,@function doesn't help
- .func/.endfunc don't make a difference either
- turning off debug fission also doesn't help

I've timed out looking into GDB internals.

> If so, we might want to emit minimal assembly stub here via asm volatile
> instead of inserting unused function body. I think it may express intention
> more clearly even for GCC case, and should work with Clang as well.

Agreed, but this seems orthogonal to this issue.

[Angelos Oikonomopoulos]

(In reply to Konstantin Tokarev from comment #22)
> Wouldn't it be enough to define NO_REORDER as empty for non-gcc? Yes clang
> will be able to move definition of marker, but maybe gdb can be satisfied by
> having entry point somewhere, not in exact location?

I should add that this is a problem with GDB only. So, if one is using clang, they can hopefully use LLDB as well, which doesn't have any trouble finding the line information without the "marker" symbols.

(though of course that's not much help if one needs a feature that exists in GDB but not LLDB :/).

[Konstantin Tokarev]

>So, if one is using clang, they can hopefully use LLDB as well

It's not always the case that person building WebKit and using debugger are the same people, e.g. in case of binary distribution, but it's certainly easier to change debugger than compiler in that case :)

[Angelos Oikonomopoulos]

Right. The larger point remains that
- I don't see an easy fix for this when compiling with clang
- I don't see a solution for GDB other than having marker functions

Would gating these marker symbols on COMPILER(GDB) be acceptable as things are?
If not, would defining away MARKER_SYMBOL work when compiling with clang be preferable?

Any other suggestion welcome.

[Angelos Oikonomopoulos]

Minimal bug report submitted upstream: https://sourceware.org/bugzilla/show_bug.cgi?id=25730

[Angelos Oikonomopoulos]

So, to summarize:

- clang + gdb is currently a no-go. I don't see a workaround for this ATM, so this would need to be fixed upstream in GDB. Have opened an issue on their bugzilla. In any case, it'll be quite some time before the fix is available in the toolchains that people use.
- gcc + gdb should work with this patch.

The only downside I can see so far is the extra #if COMPILER(GCC) directives for the MARKER_SYMBOL. I could remove those by making MARKER_SYMBOL a no-op on !GCC. My preference is for the former, as it's more explicit, but can easily submit the latter approach too.

Would be great to have this upstream, so looking forward to fixing any remaining issues.

[Carlos Alberto Lopez Perez]

(In reply to Angelos Oikonomopoulos from comment #29)
> So, to summarize:
> 
> - clang + gdb is currently a no-go. I don't see a workaround for this ATM,
> so this would need to be fixed upstream in GDB. Have opened an issue on
> their bugzilla. In any case, it'll be quite some time before the fix is
> available in the toolchains that people use.

Thanks for the reporting the issue upstream!

> - gcc + gdb should work with this patch.
> 
> The only downside I can see so far is the extra #if COMPILER(GCC) directives
> for the MARKER_SYMBOL. I could remove those by making MARKER_SYMBOL a no-op
> on !GCC. My preference is for the former, as it's more explicit, but can
> easily submit the latter approach too.
> 
> Would be great to have this upstream, so looking forward to fixing any
> remaining issues.

It looks the Windows EWS failed to build this last version of the patch, if you can take a look to it, that would be great.

[Angelos Oikonomopoulos]

Created attachment 395029 [details]
Patch

[Angelos Oikonomopoulos]

Created attachment 395039 [details]
Patch

[Angelos Oikonomopoulos]

Created attachment 395053 [details]
Patch

Testing windows build on EWS. Ignore

[Darin Adler]

Comment on attachment 395039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395039&action=review

> Source/WTF/wtf/Compiler.h:388
> +/* NO_REORDER */
> +#if !defined(NO_REORDER) && COMPILER(GCC)
> +#define NO_REORDER __attribute__((__no_reorder__))
> +#endif

Inconsistent to include a comment for NO_REORDER and not on MARKER_SYMBOL.

Also, all the other comments (except for UNUSED_VARIABLE) have blank lines after them.

Overkill to define a macro for NO_REORDER and use that macro in only one place, also under a COMPILE(GCC) conditional. I would not add this macro.

[Angelos Oikonomopoulos]

Created attachment 395166 [details]
Patch

Testing windows build on EWS. Ignore

[Angelos Oikonomopoulos]

Created attachment 395174 [details]
Patch

[Angelos Oikonomopoulos]

(In reply to Darin Adler from comment #34)
> Comment on attachment 395039 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395039&action=review
> 
> > Source/WTF/wtf/Compiler.h:388
> > +/* NO_REORDER */
> > +#if !defined(NO_REORDER) && COMPILER(GCC)
> > +#define NO_REORDER __attribute__((__no_reorder__))
> > +#endif
> 
> Inconsistent to include a comment for NO_REORDER and not on MARKER_SYMBOL.
> 
> Also, all the other comments (except for UNUSED_VARIABLE) have blank lines
> after them.
> 
> Overkill to define a macro for NO_REORDER and use that macro in only one
> place, also under a COMPILE(GCC) conditional. I would not add this macro.

Thanks. Dropped the NO_REORDER macro (including comment). Should now build properly on windows too.

[Darin Adler]

Comment on attachment 395174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395174&action=review

Looks fine to me; please consider further refinement to the Compiler.h macro

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:549
> +#endif /* WTF_COMPILER_GCC */

Code style nitpick:

This isn’t following the coding style of the rest of this file and WebKit. We aren’t commenting every #endif, particularly when a block of code is this short. And we use "//" comments when we do. And we don’t comment with the underlying macro name instead of the actual expression on the #if statement.

Better to not have an #if here at all (see comment in Compiler.h).

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:557
> +#endif /* WTF_COMPILER_GCC */

Ditto.

> Source/WTF/wtf/Compiler.h:388
> +#if !defined(MARKER_SYMBOL) && COMPILER(GCC)
> +#define MARKER_SYMBOL(name) \
> +    __attribute__((__no_reorder__)) void name(void) { __asm__(""); }
> +#endif

It’s more consistent with the Compiler.h compiler-independence strategy to define MARKER_SYMBOL everywhere and have it be a no-op on compilers that don’t need it. Might also need a more unambiguous name. The phrase "marker symbol" doesn’t point straight at a compiler capability to me. Seems a bit obscure to claim such a basic name. I might have chosen a longer name like DEBUGGER_ANNOTATION_MARKER(name). If we want to emphasize that it’s GCC-specific can even include GCC in the name.

[Angelos Oikonomopoulos]

Created attachment 395262 [details]
Patch

[Angelos Oikonomopoulos]

Switched to DEBUGGER_ANNOTATION_MARKER as suggested, made it a no-op on !GCC. Style should be consistent with the rest of Compiler.h

Thanks!

[EWS]

Committed r259390: <https://trac.webkit.org/changeset/259390>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395262 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61211811>

[Yury Semikhatsky]

(In reply to EWS from comment #41)
> Committed r259390: <https://trac.webkit.org/changeset/259390>
> 

This broke compilation (with clang FWIW) on GTK:

CMake Error at Source/cmake/WebKitMacros.cmake:172 (target_link_libraries):
  Target "LowLevelInterpreterLib" of type OBJECT_LIBRARY may not be linked
  into another target.  One may link only to STATIC or SHARED libraries, or
  to executables with the ENABLE_EXPORTS property set.
Call Stack (most recent call first):
  Source/cmake/WebKitMacros.cmake:184 (_WEBKIT_TARGET)
  Source/JavaScriptCore/CMakeLists.txt:1401 (WEBKIT_FRAMEWORK)


Is there a workaround for the error?

[Konstantin Tokarev]

Comment on attachment 395262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395262&action=review

> Source/JavaScriptCore/CMakeLists.txt:1393
> +list(APPEND JavaScriptCore_LIBRARIES LowLevelInterpreterLib)

This should be JavaScriptCore_PRIVATE_LIBRARIES

[Fujii Hironori]

WinCairo clean builds get broken since this change.
It seems that LowLevelInterpreter doesn't know WTF headers dir.

> [277/5311] Building CXX object Source\JavaScriptCore\CMakeFiles\LowLevelInterpreterLib.dir\llint\LowLevelInterpreter.cpp.obj
> FAILED: Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.obj 
> C:\PROGRA~1\LLVM\bin\clang-cl.exe  /nologo -TP -DBUILDING_WITH_CMAKE=1 -DHAVE_CONFIG_H=1 -DNOCRYPT -DNOMINMAX -DUNICODE -DWINVER=0x601 -DWTF_PLATFORM_WIN_CAIRO=1 -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -D_WIN32_WINNT=0x601 -D_WINDOWS -D_WINSOCKAPI_="" -I..\..\WebKitLibraries\win\include -IJavaScriptCore\Headers -I. -I..\..\Source\JavaScriptCore -I..\..\Source\JavaScriptCore\API -I..\..\Source\JavaScriptCore\assembler -I..\..\Source\JavaScriptCore\b3 -I..\..\Source\JavaScriptCore\b3\air -I..\..\Source\JavaScriptCore\bindings -I..\..\Source\JavaScriptCore\builtins -I..\..\Source\JavaScriptCore\bytecode -I..\..\Source\JavaScriptCore\bytecompiler -I..\..\Source\JavaScriptCore\dfg -I..\..\Source\JavaScriptCore\disassembler -I..\..\Source\JavaScriptCore\disassembler\ARM64 -I..\..\Source\JavaScriptCore\disassembler\udis86 -I..\..\Source\JavaScriptCore\domjit -I..\..\Source\JavaScriptCore\ftl -I..\..\Source\JavaScriptCore\heap -I..\..\Source\JavaScriptCore\debugger -I..\..\Source\JavaScriptCore\inspector -I..\..\Source\JavaScriptCore\inspector\agents -I..\..\Source\JavaScriptCore\inspector\augmentable -I..\..\Source\JavaScriptCore\inspector\remote -I..\..\Source\JavaScriptCore\interpreter -I..\..\Source\JavaScriptCore\jit -I..\..\Source\JavaScriptCore\llint -I..\..\Source\JavaScriptCore\parser -I..\..\Source\JavaScriptCore\profiler -I..\..\Source\JavaScriptCore\runtime -I..\..\Source\JavaScriptCore\tools -I..\..\Source\JavaScriptCore\wasm -I..\..\Source\JavaScriptCore\wasm\js -I..\..\Source\JavaScriptCore\yarr -IJavaScriptCore\DerivedSources -IJavaScriptCore\DerivedSources\inspector -IJavaScriptCore\DerivedSources\runtime -IJavaScriptCore\DerivedSources\yarr -I..\include\private -I..\..\Source\JavaScriptCore\inspector\remote\socket -IWTF\Headers -IDerivedSources -I..\..\Source\ThirdParty /W4 -fdiagnostics-color=always -fcolor-diagnostics -Wno-noexcept-type -Wno-parentheses-equality -Qunused-arguments -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-unknown-argument -Wno-nonportable-include-path -Wno-unknown-pragmas -Wno-macro-redefined -Wno-undef /DWIN32 /D_WINDOWS  /GR- /EHsc- -fno-strict-aliasing /MD /O2 /Ob2 /DNDEBUG   /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189 /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305 /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459 /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4646 /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011 /wd6031 /wd6211 /wd6246 /wd6255 /wd6387 /wd4091 /Zi /GS /EHa- /EHc- /EHs- /fp:except- /analyze- /bigobj /utf-8 /validate-charset /Oy- -fmsc-version=1911 -std:c++17 /showIncludes /FoSource\JavaScriptCore\CMakeFiles\LowLevelInterpreterLib.dir\llint\LowLevelInterpreter.cpp.obj /FdSource\JavaScriptCore\CMakeFiles\LowLevelInterpreterLib.dir\ -c ..\..\Source\JavaScriptCore\llint\LowLevelInterpreter.cpp
> In file included from ..\..\Source\JavaScriptCore\llint\LowLevelInterpreter.cpp:26:
> In file included from ..\..\Source\JavaScriptCore\config.h:33:
> In file included from ..\..\Source\JavaScriptCore\runtime\JSExportMacros.h:32:
> WTF\Headers\wtf/ExportMacros.h(32,10): fatal error: 'wtf/Platform.h' file not found
> #include <wtf/Platform.h>
>          ^~~~~~~~~~~~~~~~
> 1 error generated.

[Stephan Szabo]

And for a clean build of wincairo after adding a dependency on WTF_CopyHeaders to try to get the WTF headers, I seem to see a failure looking for Bytecodes.h, possibly because it isn't generated from the custom command yet, so the object library may need a dependency on that as well.

D:\github\webkit\Source\JavaScriptCore\bytecode\Opcode.h(32): fatal error C1083: Cannot open include file: 'Bytecodes.h': No such file or directory

[Fujii Hironori]

WinCairo
https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Debug%20%28Build%29/builds/15707

[Angelos Oikonomopoulos]

Created attachment 395371 [details]
Patch

[Angelos Oikonomopoulos]

(In reply to Konstantin Tokarev from comment #44)
> Comment on attachment 395262 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395262&action=review
> 
> > Source/JavaScriptCore/CMakeLists.txt:1393
> > +list(APPEND JavaScriptCore_LIBRARIES LowLevelInterpreterLib)
> 
> This should be JavaScriptCore_PRIVATE_LIBRARIES

According to the cmake-3.10 docs (https://cmake.org/cmake/help/v3.10/command/add_library.html#object-libraries), you're not supposed to use target_link_libraris with OBJECT libraries (this limitation has since been lifted, see https://gitlab.kitware.com/cmake/cmake/-/issues/14778).

With this change (in the newer patch, https://bugs.webkit.org/attachment.cgi?id=395371), I can no longer reproduce the issue reported in comment 43.

Still need to reproduce the WinCairo issue.

[Angelos Oikonomopoulos]

(In reply to Angelos Oikonomopoulos from comment #49)
> With this change (in the newer patch,
> https://bugs.webkit.org/attachment.cgi?id=395371), I can no longer reproduce
> the issue reported in comment 43.

To be clear, the relevant change is:

-list(APPEND JavaScriptCore_LIBRARIES LowLevelInterpreterLib)
+list(APPEND JavaScriptCore_SOURCES $<TARGET_OBJECTS:LowLevelInterpreterLib>)

[Konstantin Tokarev]

Yes, my bad, your change looks correct.

[Angelos Oikonomopoulos]

Created attachment 395790 [details]
Patch

[Angelos Oikonomopoulos]

Reproduced the issue on wincairo and the new patch seems to fix it on my setup.

[Konstantin Tokarev]

Comment on attachment 395790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395790&action=review

> Source/JavaScriptCore/CMakeLists.txt:1375
> +add_dependencies(LowLevelInterpreterLib WTF_CopyHeaders)

List all dependencies of LowLevelInterpreterLib in one add_dependencies() invocation.

> Source/JavaScriptCore/CMakeLists.txt:1376
> +add_dependencies(LowLevelInterpreterLib Bytecodes)

Wouldn't it just work if you add "${JavaScriptCore_DERIVED_SOURCES_DIR}/Bytecodes.h" to dependencies instead of creating proxy Bytecodes target?

[Angelos Oikonomopoulos]

Created attachment 395796 [details]
Patch

[Angelos Oikonomopoulos]

(In reply to Konstantin Tokarev from comment #54)
> Comment on attachment 395790 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395790&action=review
> 
> > Source/JavaScriptCore/CMakeLists.txt:1375
> > +add_dependencies(LowLevelInterpreterLib WTF_CopyHeaders)
> 
> List all dependencies of LowLevelInterpreterLib in one add_dependencies()
> invocation.

Done, thanks.

> > Source/JavaScriptCore/CMakeLists.txt:1376
> > +add_dependencies(LowLevelInterpreterLib Bytecodes)
> 
> Wouldn't it just work if you add
> "${JavaScriptCore_DERIVED_SOURCES_DIR}/Bytecodes.h" to dependencies instead
> of creating proxy Bytecodes target?

Unfortunately not, cmake fails with

The dependency target ".../Bytecodes.h" of target "LowLevelInterpreterLib" does not exist.

[Konstantin Tokarev]

Comment on attachment 395796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395796&action=review

> Source/JavaScriptCore/Scripts/postprocess-asm:193
> +exit(0)

I guess this is not needed, script will normally exit with code 0 anyway

[Angelos Oikonomopoulos]

Created attachment 395811 [details]
Patch

Remove redundant exit(0)

[EWS]

Committed r259734: <https://trac.webkit.org/changeset/259734>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395811 [details].

[Angelos Oikonomopoulos]

This broke the build on jsc-i386:

https://ews-build.webkit.org/#/builders/27/builds/10323

[ 38%] Building CXX object Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o
In file included from /home/ews/worker/JSC-i386-32bits-EWS/build/Source/JavaScriptCore/runtime/JSGlobalObjectInlines.h:33,
                 from /home/ews/worker/JSC-i386-32bits-EWS/build/Source/JavaScriptCore/runtime/JSCInlines.h:47,
                 from /home/ews/worker/JSC-i386-32bits-EWS/build/Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:41:
/home/ews/worker/JSC-i386-32bits-EWS/build/Source/JavaScriptCore/bytecode/LinkTimeConstant.h:28:10: fatal error: JSCBuiltins.h: No such file or directory
 #include "JSCBuiltins.h"
          ^~~~~~~~~~~~~~~
compilation terminated.
make[3]: *** [Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/build.make:127: Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/llint/LowLevelInterpreter.cpp.o] Error 1
make[2]: *** [CMakeFiles/Makefile2:280: Source/JavaScriptCore/CMakeFiles/LowLevelInterpreterLib.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:624: Source/JavaScriptCore/shell/CMakeFiles/jsc.dir/rule] Error 2
make: *** [Makefile:340: jsc] Error 2

[Angelos Oikonomopoulos]

Created attachment 395941 [details]
Patch

Fix build failure on jsc-i386 (i.e. CLOOP)

[Angelos Oikonomopoulos]

Latest patch should fix the build failure on --cloop builds. What's more, I went through all other instances of add_custom_command again and the cross referenced with the output of cpp -M for LowLevelInterpreter.cpp , so hopefully this should be the last fix needed. The first time through, I'd missed JSCBuiltins.h as it extended to the far right of the OUTPUT line.

[EWS]

Committed r259792: <https://trac.webkit.org/changeset/259792>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395941 [details].