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

erts: Apply -fno-omit-frame-pointer to erts/lib_src when JIT is enabled #6048

Merged

Conversation

d1jang
Copy link

@d1jang d1jang commented Jun 3, 2022

Currently OMIT_OMIT_FP is set to true in erts/emulator/Makefile.in
when JIT is enabled, but not necessarily in erts/lib_src/Makefile.in.

This change factors out OMIT_OMIT_FP computation to be shared between
those two builds to apply this flag consistently to them.

Note that this bug was confirmed by @garazdawi in an Erlang forum post: https://erlangforums.com/t/unify-builds-for-otp-erts-targets/1419/3

@CLAassistant
Copy link

CLAassistant commented Jun 3, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2022

CT Test Results

       4 files     135 suites   51m 20s ⏱️
1 611 tests 1 560 ✔️ 51 💤 0
2 001 runs  1 932 ✔️ 69 💤 0

Results for commit 1bc6daf.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@garazdawi
Copy link
Contributor

Hello! Thanks for the PR! Would you mind signing the CLA?

@garazdawi garazdawi self-assigned this Jun 7, 2022
@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Jun 7, 2022
@d1jang
Copy link
Author

d1jang commented Jun 7, 2022

Hello! Thanks for the PR! Would you mind signing the CLA?

Done!

Copy link
Contributor

@garazdawi garazdawi left a comment

Choose a reason for hiding this comment

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

You need to also change erts/configure.ac to include flavor.mk.in as erts/configure is a generated file.

@garazdawi garazdawi added the waiting waiting for changes/input from author label Jun 17, 2022
@d1jang d1jang requested a review from garazdawi June 23, 2022 03:54
@d1jang
Copy link
Author

d1jang commented Jun 23, 2022

You need to also change erts/configure.ac to include flavor.mk.in as erts/configure is a generated file.

Thanks for the comment! I included the requested change in [erts/configure.ac].

@d1jang
Copy link
Author

d1jang commented Jul 12, 2022

@garazdawi I made the requested change. Can I ask for a quick review? Thanks!

@RaimoNiskanen
Copy link
Contributor

@d1jang: I am afraid @garazdawi is on vacation until mid August. Maybe someone else in the team can find the time to have a look, but do not hold your breath...

@garazdawi garazdawi added testing currently being tested, tag is used by OTP internal CI and removed waiting waiting for changes/input from author testing currently being tested, tag is used by OTP internal CI labels Aug 26, 2022
@garazdawi
Copy link
Contributor

Hello again! The changes in this PR break the building of a non-jit emulator. To reproduce run:

./otp_build setup -a
make FLAVOR=emu

@garazdawi garazdawi force-pushed the apply_no_omit_frame_pointer_to_erts_lib_src branch 2 times, most recently from 91f55a2 to 1bc6daf Compare October 4, 2022 12:27
@garazdawi
Copy link
Contributor

I've pushed changes to your branch to fix the issues and add CI in order to check that the different types and flavors works.

@garazdawi garazdawi added the testing currently being tested, tag is used by OTP internal CI label Oct 4, 2022
@garazdawi garazdawi force-pushed the apply_no_omit_frame_pointer_to_erts_lib_src branch from 1bc6daf to 73a7588 Compare October 5, 2022 06:44
@garazdawi garazdawi merged commit 3072f9b into erlang:master Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants