Skip to content

Conversation

justinmichaud
Copy link
Contributor

@justinmichaud justinmichaud commented Sep 7, 2024

@justinmichaud justinmichaud requested a review from a team as a code owner September 7, 2024 17:02
@justinmichaud justinmichaud self-assigned this Sep 7, 2024
@justinmichaud justinmichaud added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Sep 7, 2024
Comment on lines 419 to 420
Copy link
Contributor

@mcatanzaro mcatanzaro Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, a few observations:

  • OptionsGTK.cmake would need the same change
  • You really don't want to add ${CMAKE_COMPILER_SIZE_OPT_FLAGS} here. That's surely going to override whatever the desired optimization level is with -OS
  • All major distros already build with -fasynchronous-unwind-tables. Do you know how adding -funwind-tables will interact with -fasynchronous-unwind-tables? I don't know what will happen, but I fear -funwind-tables may sabotage -fasynchronous-unwind-tables because you are appending it after the distro-provided flags.

Here are my suggestions:

  • Add the flag in WebKitCompilerFlags.cmake instead of here. It's OK to do so even when not building with libbacktrace.
  • Don't mess with size optimization flags, since that's unrelated
  • Use -fasynchronous-unwind-tables since that's what all distros do; presumably libbacktrace is compatible with that? Also, prepend this flag rather than appending it, since there's no reason to prevent overriding it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would probably add this where -fno-exceptions is added, as this is only needed at all due to that

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 7, 2024
@justinmichaud justinmichaud force-pushed the eng/Use--funwind-tables-when-LIBBACKTRACE-is-enabled- branch from cb8c52e to a67ff7d Compare September 8, 2024 22:01
@justinmichaud justinmichaud force-pushed the eng/Use--funwind-tables-when-LIBBACKTRACE-is-enabled- branch from a67ff7d to fbd921a Compare September 9, 2024 16:54
Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also update the commit message before landing, since you're now using the flag always (which is fine) rather than only when LIBBACKTRACE is enabled.

@justinmichaud justinmichaud removed the merging-blocked Applied to prevent a change from being merged label Sep 9, 2024
@justinmichaud justinmichaud force-pushed the eng/Use--funwind-tables-when-LIBBACKTRACE-is-enabled- branch from fbd921a to 4c06a87 Compare September 9, 2024 19:42
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 9, 2024
@justinmichaud justinmichaud changed the title Use -funwind-tables when LIBBACKTRACE is enabled. Always use -fasynchronous-unwind-table. Sep 10, 2024
@justinmichaud justinmichaud added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Sep 10, 2024
https://bugs.webkit.org/show_bug.cgi?id=279300

Reviewed by Michael Catanzaro.

This option is needed on ARMv7 for libbacktrace to work at all. The
downstream port of WPE already has a similar patch, but it should be
upstreamed too.

* Source/cmake/OptionsWPE.cmake:

Canonical link: https://commits.webkit.org/283378@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Use--funwind-tables-when-LIBBACKTRACE-is-enabled- branch from 4c06a87 to 7db7c2a Compare September 10, 2024 00:47
@webkit-commit-queue
Copy link
Collaborator

Committed 283378@main (7db7c2a): https://commits.webkit.org/283378@main

Reviewed commits have been landed. Closing PR #33279 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 7db7c2a into WebKit:main Sep 10, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Sep 10, 2024
@justinmichaud justinmichaud deleted the eng/Use--funwind-tables-when-LIBBACKTRACE-is-enabled- branch September 10, 2024 02:13
@aperezdc
Copy link
Contributor

Backported into the webkitglib/2.44 branch as commit 82bc1d5 and into the webkitglib/2.46 branch as b19ec37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants