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

Improve embedded PCK loading and exporting. #56093

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Dec 20, 2021

[Windows export]

  • Limit total executable size to 4 GB - 1.
  • Use "rcedit" before embedding PCK.
  • Capture and process "rcedit" errors.

Fixes #56051

[Windows and Linux] Add support for PCK loading from executable "pck" section.

  • Exported projects now read back executable section information and attempt to load PCK from the section in addition to looking at the end of the file. This allows to modify and sign executable after embedding PCK.

Note: need testing with different build configurations, some might strip "pck" section stub and prevent it from loading.

Fixes #32310, fixes #33466.

@bruvzg
Copy link
Member Author

bruvzg commented Dec 21, 2021

Some extra info, it's improving the situation with the embedded PCK a bit, but not fixing it:

  • 2 GB is a limit for the memory image, max. executable size seems to 4 GB - 1 (in total, not a section size).
  • It's not working with rcedit, which seems to mess up section information, either we are not updating it correctly or it's rcedit issue. Also, seems like it's incapable of updating VS 2022 generated binaries with LTO at all.
  • VS seems to strip "pck" when LTO is enabled, but it can be fixed with an extra hack.
  • Works with code signing.

I'll try to apply rcedit before embedding the pack as the workaround.

Tested with following configurations:

  • VS 2022 (7.0.4) - 64-bit release with LTO, 64-bit release without LTO, 32-bit release with LTO, 32-bit release without LTO.
  • MSYS2/GCC (11.2.0-rev2) - 64-bit release with LTO, 64-bit release without LTO, 32-bit release with LTO, 32-bit release without LTO.
  • MSYS2/LLVM (13.0.0) - 64-bit release without LTO (have not tested 32-bit, build with LTO fails to link).

@bruvzg
Copy link
Member Author

bruvzg commented Dec 21, 2021

I'll try to apply rcedit before embedding the pack as the workaround.

This seems to work.

@bruvzg bruvzg marked this pull request as ready for review February 2, 2022 07:58
@bruvzg bruvzg requested review from a team as code owners February 2, 2022 07:58
@bruvzg bruvzg force-pushed the pck_section_load branch 4 times, most recently from c89e586 to 62e529f Compare March 17, 2022 06:16
@bruvzg bruvzg force-pushed the pck_section_load branch from 62e529f to fb3a677 Compare March 29, 2022 20:21
Windows export process:
  Limit size of executable with embedded PCK to 4 GB.
  Use "rcedit" before embedding PCK.
  Capture and process "rcedit" errors.

Windows, Linux:
  Add support for PCK loading from executable "pck" section.
@bruvzg bruvzg force-pushed the pck_section_load branch from fb3a677 to c0cc41d Compare April 20, 2022 08:10
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

I don't see (or should I say 'smell'?) anything fishy.

@akien-mga akien-mga merged commit 504708a into godotengine:master Apr 27, 2022
@akien-mga
Copy link
Member

Thanks!

@bruvzg bruvzg deleted the pck_section_load branch April 27, 2022 12:30
@akien-mga
Copy link
Member

@bruvzg Would you be up for backporting these changes to 3.x?

@bruvzg
Copy link
Member Author

bruvzg commented Apr 27, 2022

Would you be up for backporting these changes to 3.x?

Backporting should be mostly straightforward, I'll take a look tomorrow.

@bruvzg
Copy link
Member Author

bruvzg commented Apr 28, 2022

3.x version - #60580

@nathanfranke
Copy link
Contributor

strip still breaks projects with Embedded PCK. How can we work around this?

$ ./test.x86_64 
Godot Engine v4.0.alpha.custom_build.bc7ccc909 - https://godotengine.org
Vulkan API 1.2.0 - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 960
 
$ strip test.x86_64 
$ ./test.x86_64 
ERROR: Corrupted header in binary project.binary (not ECFG).
   at: _load_settings_binary (core/config/project_settings.cpp:623)
ERROR: Couldn't load file 'res://project.binary', error code 16.
   at: _load_settings_text_or_binary (core/config/project_settings.cpp:710)
Error: Couldn't load project data at path ".". Is the .pck file missing?
If you've renamed the executable, the associated .pck file should also be renamed to match the executable's name (without the extension).

@akien-mga
Copy link
Member

You should strip the template before exporting, so you don't need to strip it after. Otherwise you can maybe use strip --keep-section pck (untested).

@nathanfranke
Copy link
Contributor

For the record, strip --keep-section pck did not work for me, but stripping before exporting seems like a much better idea anyways.

I did actually figure out a solution to my underlying use case: makepkg automatically strips binaries when you install them, but you can work around it by adding options=(!strip) to PKGBUILD (Source).

@realkotob
Copy link
Contributor

There should be info about this size limit in the documentation, not sure if there's a warning somewhere to explain what's happening.

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