SCons: Add -ffile-prefix-map GCC/Clang option to make debug file paths relative
#78232
Conversation
SConstruct
Outdated
| @@ -81,7 +81,7 @@ for x in sorted(glob.glob("platform/*")): | |||
| continue | |||
| tmppath = "./" + x | |||
|
|
|||
| sys.path.insert(0, tmppath) | |||
| sys.path.insert(1, tmppath) | |||
There was a problem hiding this comment.
I changed these to be able to rely on sys.path[0] to get the project folder where SConstruct is. Yet again this seemingly unsolved Python problem to get easy access to the main working directory :P
Another option is to redo the same that @lawnjelly did in scu_builders.py:
from pathlib import Path
from os.path import normpath, basename
base_folder_path = str(Path(__file__).parent) + "/"
base_folder_only = os.path.basename(os.path.normpath(base_folder_path))Both options feel quite hacky. Feedback welcome from Python experts :)
There was a problem hiding this comment.
CC @touilleMan if you have any suggestion.
There was a problem hiding this comment.
sys.path.insert(0, tmppath) will make sure the import just below is always the right one putting it on second place means that it will check the path only after checking the local folder. if we are sure there will not be any file that could be misinterpreted as one of the libraries this should be good to go. On the other hand relying on sys.path[0] feels strange. Normally to get the project folder I would set an env variable from the main script using file and get the env var from all other scripts.
Beware I'm not familiar with godot codebase specificity hence giving advice based on quickly looking at the files which means I might (and most likely) have missed something.
There was a problem hiding this comment.
I agree with @ajreckof, changing the insert order risks causing name clash issues. It's already a hack changing the path order the way we do, if we start messing with it because other things rely on some random thing being in a certain place in the path array, it's only going to be worse.
Scons provides objects that resolve paths to the root - Dir("#").abspath or File("#nameoffile").abspath. If a script doesn't have access to these Scons functions, then having the path to the root available from an env var, like @ajreckof suggested, would be a great fallback.
There was a problem hiding this comment.
Thanks both for the advice! Both options sound good. I don't know why I didn't think of Dir("#") since we're in SConstruct and it's available... I tested it and it works, so we don't need to hack sys.path further.
| @@ -592,6 +592,9 @@ if selected_platform in platform_list: | |||
| env.Append(CCFLAGS=["-g3"]) | |||
| else: | |||
| env.Append(CCFLAGS=["-g2"]) | |||
| # Remap absolute paths to relative paths for debug symbols. | |||
| project_path = sys.path[0] | |||
| env.Append(CCFLAGS=[f"-ffile-prefix-map={project_path}=."]) | |||
There was a problem hiding this comment.
For the record I tried both with =. and just =, both work fine but there's no difference, relative paths still start with ./.
akien-mga
force-pushed
the
scons-debug-ffile-prefix-map
branch
from
597368c
to
2630a47
Compare
Added a second commit implementing this, to be squashed if approved. The behavior can now be disabled with the One "drawback" of using this feature is that the pwd will appear in the build logs: But that's what needed for it to not appear in the actual binary's symbols (my |
| # Set optimize and debug_symbols flags. | ||
| # "custom" means do nothing and let users set their own optimization flags. | ||
| # Needs to happen after configure to have `env.msvc` defined. | ||
| if env.msvc: | ||
| if env["debug_symbols"]: | ||
| env.Append(CCFLAGS=["/Zi", "/FS"]) | ||
| env.Append(LINKFLAGS=["/DEBUG:FULL"]) | ||
| else: | ||
| env.Append(LINKFLAGS=["/DEBUG:NONE"]) |
There was a problem hiding this comment.
Technically I just moved the compiler version checks before this stuff, but Git shows it the other way around ;)
| if env["debug_paths_relative"] and cc_version_major < 10: | ||
| print("Clang < 10 doesn't support -ffile-prefix-map, disabling `debug_paths_relative` option.") | ||
| env["debug_paths_relative"] = False |
There was a problem hiding this comment.
To avoid repetition I only check the not vanilla case in the macOS/iOS branch, which should already gracefully handle disabling the option if Apple Clang version is too old. So I think it should be fine with this check re-running for vanilla Clang in the main scope.
The same reasoning could likely be adopted for the repeated Clang 6 check.
|
Confirmed the editor builds and runs fine with this PR on VS2022 MSVC. |
|
The MSVC equivalents seem to be Compare: |
akien-mga
force-pushed
the
scons-debug-ffile-prefix-map
branch
from
b5a314f
to
74e6b5a
Compare
|
So I noticed an issue caused by this, which is this warning in gdb: Since the paths are relative, the source files can only be found when running Godot from the root of the source repository folder. So running Godot from a project folder with While the backtrace still looks the same, inspecting frames with So for local development builds, we should probably default this new option to False and keep the previous behavior. The |
As pointed out in godotengine#78232 (comment), it actually makes it harder to run Godot locally while keeping the relationship with the header files it was compiled from.
As pointed out in godotengine#78232 (comment), it actually makes it harder to run Godot locally while keeping the relationship with the header files it was compiled from.
As pointed out in godotengine#78232 (comment), it actually makes it harder to run Godot locally while keeping the relationship with the header files it was compiled from.
As pointed out in godotengine#78232 (comment), it actually makes it harder to run Godot locally while keeping the relationship with the header files it was compiled from.
As pointed out in godotengine#78232 (comment), it actually makes it harder to run Godot locally while keeping the relationship with the header files it was compiled from.
Supported since GCC 8 and Clang 10.
I need to add checks to actually enforce that we only define it for compatible versions, or just add a SCons option to toggle this on or off (which might be needed for distro packaging as those typically remap debuginfo paths to where they install them system wide).
Follow-up to #78063.
Before this PR:
With this PR:
There's still this pesky
./prefix but it might come from something else, as it's also present unnecessarily in the "before" path:/home/akien/Projects/godot/godot.git/./core/core_bind.cpp:240I'd appreciate help from interesting contributors who want to review the actual impact of the various
-f*-prefix-mapflags (there's quite a bunch of them) and best practices when it comes to making reproducible builds and avoiding including buildsystem specific paths in the binary and debuginfo.