Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools: disable LTO for "v8_cppgc_shared" target #38346

Closed
wants to merge 1 commit into from
Closed

tools: disable LTO for "v8_cppgc_shared" target #38346

wants to merge 1 commit into from

Conversation

jesec
Copy link
Member

@jesec jesec commented Apr 22, 2021

"PushAllRegistersAndIterateStack" is implemented in assembly and
called from "stack.cc" via 'extern "C"'. [1]

However, LTO does not work well with symbol usage from assembly. [2]

This change workarounds the issue by disabling LTO for the target.

With GCC 10 and "./configure --enable-lto", compilation succeeds
after this change.

[1] v8/v8@c10863153
[2] https://gcc.gnu.org/wiki/LinkTimeOptimizationFAQ#Symbol_usage_from_assembly_language

Refs: #35957
Refs: #38335
Signed-off-by: Jesse Chan jc@linux.com

"PushAllRegistersAndIterateStack" is implemented in assembly and
called from "stack.cc" via 'extern "C"'. [1]

However, LTO does not work well with symbol usage from assembly. [2]

This change workarounds the issue by disabling LTO for the target.

With GCC 10 and "./configure --enable-lto", compilation succeeds
after this change.

[1] v8/v8@c10863153
[2] https://gcc.gnu.org/wiki/LinkTimeOptimizationFAQ#Symbol_usage_from_assembly_language

Refs: #35957
Refs: #38335
Signed-off-by: Jesse Chan <jc@linux.com>
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 22, 2021
Copy link
Contributor

@XadillaX XadillaX left a comment

Choose a reason for hiding this comment

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

lgtm if CI is OK

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

LGTM, although the conditional is probably redundant.

@@ -1378,6 +1378,9 @@
'<(V8_ROOT)/src/heap/base/worklist.h',
],
'conditions': [
['enable_lto=="true"', {
'cflags_cc': [ '-fno-lto' ],
Copy link
Member

Choose a reason for hiding this comment

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

This could be set regardless of the enable_lto setting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically, yes, at least for GCC. But I am not sure about MSVC or Clang. Since enable_lto implies some other conditions, I think it is better to have this conditional.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@gengjiawen
Copy link
Member

Landed in 6ca785b

gengjiawen pushed a commit that referenced this pull request Apr 29, 2021
"PushAllRegistersAndIterateStack" is implemented in assembly and
called from "stack.cc" via 'extern "C"'. [1]

However, LTO does not work well with symbol usage from assembly. [2]

This change workarounds the issue by disabling LTO for the target.

With GCC 10 and "./configure --enable-lto", compilation succeeds
after this change.

[1] v8/v8@c10863153
[2] https://gcc.gnu.org/wiki/LinkTimeOptimizationFAQ#Symbol_usage_from_assembly_language

Refs: #35957
Refs: #38335
Signed-off-by: Jesse Chan <jc@linux.com>

PR-URL: #38346
Refs: #35957
Refs: #38335
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@gengjiawen gengjiawen closed this Apr 29, 2021
targos pushed a commit that referenced this pull request Apr 29, 2021
"PushAllRegistersAndIterateStack" is implemented in assembly and
called from "stack.cc" via 'extern "C"'. [1]

However, LTO does not work well with symbol usage from assembly. [2]

This change workarounds the issue by disabling LTO for the target.

With GCC 10 and "./configure --enable-lto", compilation succeeds
after this change.

[1] v8/v8@c10863153
[2] https://gcc.gnu.org/wiki/LinkTimeOptimizationFAQ#Symbol_usage_from_assembly_language

Refs: #35957
Refs: #38335
Signed-off-by: Jesse Chan <jc@linux.com>

PR-URL: #38346
Refs: #35957
Refs: #38335
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@targos targos mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants