Skip to content

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.

],
'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