Bug 207119 - Enable offlineasm debug annotations for GCC
Summary: Enable offlineasm debug annotations for GCC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 209944
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-03 08:29 PST by Angelos Oikonomopoulos
Modified: 2020-04-09 06:16 PDT (History)
23 users (show)

See Also:


Attachments
Patch (24.99 KB, patch)
2020-02-03 08:33 PST, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (25.51 KB, patch)
2020-03-23 10:01 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (26.62 KB, patch)
2020-03-24 07:16 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (28.49 KB, patch)
2020-03-25 10:06 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (28.59 KB, patch)
2020-03-25 10:24 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (27.29 KB, patch)
2020-03-31 06:09 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (27.21 KB, patch)
2020-03-31 07:54 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (27.35 KB, patch)
2020-03-31 09:52 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (27.74 KB, patch)
2020-04-01 07:52 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (27.74 KB, patch)
2020-04-01 09:14 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (27.81 KB, patch)
2020-04-02 06:05 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (27.83 KB, patch)
2020-04-03 06:50 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (28.84 KB, patch)
2020-04-08 04:21 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (28.80 KB, patch)
2020-04-08 06:04 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (28.79 KB, patch)
2020-04-08 08:21 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff
Patch (1.84 KB, patch)
2020-04-09 05:45 PDT, Angelos Oikonomopoulos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Angelos Oikonomopoulos 2020-02-03 08:29:30 PST
Enable offlineasm debug annotations for GCC
Comment 1 Angelos Oikonomopoulos 2020-02-03 08:33:18 PST
Created attachment 389516 [details]
Patch
Comment 2 Angelos Oikonomopoulos 2020-02-03 09:05:34 PST
r?
Comment 3 Angelos Oikonomopoulos 2020-02-18 07:25:29 PST
ping
Comment 4 Angelos Oikonomopoulos 2020-03-23 10:01:52 PDT
Created attachment 394270 [details]
Patch
Comment 5 Carlos Alberto Lopez Perez 2020-03-23 10:47:10 PDT
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})
Comment 6 Konstantin Tokarev 2020-03-24 04:19:08 PDT
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?
Comment 7 Angelos Oikonomopoulos 2020-03-24 05:15:23 PDT
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.
Comment 8 Angelos Oikonomopoulos 2020-03-24 07:16:42 PDT
Created attachment 394363 [details]
Patch
Comment 9 Konstantin Tokarev 2020-03-24 09:10:07 PDT
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} ?
Comment 10 Konstantin Tokarev 2020-03-24 09:33:50 PDT
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")?
Comment 11 Konstantin Tokarev 2020-03-24 10:20:20 PDT
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.
Comment 12 Carlos Alberto Lopez Perez 2020-03-24 12:27:22 PDT
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
Comment 13 Yusuke Suzuki 2020-03-24 15:37:02 PDT
How about using wrapped-GCC command only for LowLevelInterpreter.cpp and ensure LLIntAssembly.h is only included by this file?
Comment 14 Konstantin Tokarev 2020-03-24 16:01:34 PDT
(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.
Comment 15 Konstantin Tokarev 2020-03-24 16:26:42 PDT
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.
Comment 16 Carlos Alberto Lopez Perez 2020-03-24 20:04:17 PDT
(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
Comment 17 Angelos Oikonomopoulos 2020-03-25 10:06:51 PDT
Created attachment 394511 [details]
Patch
Comment 18 Angelos Oikonomopoulos 2020-03-25 10:07:58 PDT
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.
Comment 19 Angelos Oikonomopoulos 2020-03-25 10:24:43 PDT
Created attachment 394515 [details]
Patch
Comment 20 Angelos Oikonomopoulos 2020-03-25 10:25:47 PDT
(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.
Comment 21 Konstantin Tokarev 2020-03-25 10:54:15 PDT
(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.
Comment 22 Konstantin Tokarev 2020-03-25 10:57:03 PDT
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?
Comment 23 Konstantin Tokarev 2020-03-25 11:00:37 PDT
Actually, it seems to me that cleaner solution would be to emit cfi directives in offlineasm instead of relying on MARKER_SYMBOL hacks
Comment 24 Angelos Oikonomopoulos 2020-03-26 06:24:55 PDT
(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.
Comment 25 Angelos Oikonomopoulos 2020-03-26 07:38:01 PDT
(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 :/).
Comment 26 Konstantin Tokarev 2020-03-26 07:45:50 PDT
>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 :)
Comment 27 Angelos Oikonomopoulos 2020-03-26 08:19:34 PDT
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.
Comment 28 Angelos Oikonomopoulos 2020-03-26 08:37:43 PDT
Minimal bug report submitted upstream: https://sourceware.org/bugzilla/show_bug.cgi?id=25730
Comment 29 Angelos Oikonomopoulos 2020-03-30 07:41:22 PDT
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.
Comment 30 Carlos Alberto Lopez Perez 2020-03-30 08:22:50 PDT
(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.
Comment 31 Angelos Oikonomopoulos 2020-03-31 06:09:44 PDT
Created attachment 395029 [details]
Patch
Comment 32 Angelos Oikonomopoulos 2020-03-31 07:54:41 PDT
Created attachment 395039 [details]
Patch
Comment 33 Angelos Oikonomopoulos 2020-03-31 09:52:30 PDT
Created attachment 395053 [details]
Patch

Testing windows build on EWS. Ignore
Comment 34 Darin Adler 2020-03-31 10:39:26 PDT
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.
Comment 35 Angelos Oikonomopoulos 2020-04-01 07:52:57 PDT
Created attachment 395166 [details]
Patch

Testing windows build on EWS. Ignore
Comment 36 Angelos Oikonomopoulos 2020-04-01 09:14:37 PDT
Created attachment 395174 [details]
Patch
Comment 37 Angelos Oikonomopoulos 2020-04-01 09:55:09 PDT
(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.
Comment 38 Darin Adler 2020-04-01 10:01:36 PDT
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.
Comment 39 Angelos Oikonomopoulos 2020-04-02 06:05:11 PDT
Created attachment 395262 [details]
Patch
Comment 40 Angelos Oikonomopoulos 2020-04-02 06:07:16 PDT
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!
Comment 41 EWS 2020-04-02 09:39:11 PDT
Committed r259390: <https://trac.webkit.org/changeset/259390>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395262 [details].
Comment 42 Radar WebKit Bug Importer 2020-04-02 09:40:16 PDT
<rdar://problem/61211811>
Comment 43 Yury Semikhatsky 2020-04-02 12:25:19 PDT
(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?
Comment 44 Konstantin Tokarev 2020-04-02 12:34:42 PDT
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
Comment 45 Fujii Hironori 2020-04-02 14:25:09 PDT
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.
Comment 46 Stephan Szabo 2020-04-02 16:19:02 PDT
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
Comment 48 Angelos Oikonomopoulos 2020-04-03 06:50:19 PDT
Created attachment 395371 [details]
Patch
Comment 49 Angelos Oikonomopoulos 2020-04-03 07:30:11 PDT
(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.
Comment 50 Angelos Oikonomopoulos 2020-04-03 07:31:47 PDT
(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>)
Comment 51 Konstantin Tokarev 2020-04-03 07:33:22 PDT
Yes, my bad, your change looks correct.
Comment 52 Angelos Oikonomopoulos 2020-04-08 04:21:59 PDT
Created attachment 395790 [details]
Patch
Comment 53 Angelos Oikonomopoulos 2020-04-08 05:37:32 PDT
Reproduced the issue on wincairo and the new patch seems to fix it on my setup.
Comment 54 Konstantin Tokarev 2020-04-08 05:42:12 PDT
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?
Comment 55 Angelos Oikonomopoulos 2020-04-08 06:04:49 PDT
Created attachment 395796 [details]
Patch
Comment 56 Angelos Oikonomopoulos 2020-04-08 06:05:12 PDT
(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.
Comment 57 Konstantin Tokarev 2020-04-08 08:09:19 PDT
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
Comment 58 Angelos Oikonomopoulos 2020-04-08 08:21:40 PDT
Created attachment 395811 [details]
Patch

Remove redundant exit(0)
Comment 59 EWS 2020-04-08 11:23:57 PDT
Committed r259734: <https://trac.webkit.org/changeset/259734>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395811 [details].
Comment 60 Angelos Oikonomopoulos 2020-04-09 05:42:29 PDT
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
Comment 61 Angelos Oikonomopoulos 2020-04-09 05:45:59 PDT
Created attachment 395941 [details]
Patch

Fix build failure on jsc-i386 (i.e. CLOOP)
Comment 62 Angelos Oikonomopoulos 2020-04-09 05:47:56 PDT
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.
Comment 63 EWS 2020-04-09 06:16:36 PDT
Committed r259792: <https://trac.webkit.org/changeset/259792>

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