Skip to content

Fix debug-compilation-dir for system libs #20776

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 12, 2023
Merged

Fix debug-compilation-dir for system libs #20776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9782fbd
Fix debug-compilation-dir for system libs
kon72 Nov 24, 2023
876e853
Fix by using file-prefix-map instead
kon72 Dec 2, 2023
40e01fb
Add test
kon72 Dec 2, 2023
51b0f24
Run test on crossplatform
kon72 Dec 3, 2023
ac473ca
Skip test if FROZEN_CACHE is not set
kon72 Dec 3, 2023
5180bf3
clean
kon72 Dec 3, 2023
d927339
Use relative path for DW_AT_name
kon72 Dec 3, 2023
ef45f57
Normalize path separators of source files
kon72 Dec 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9798,6 +9798,21 @@ def test_separate_dwarf_with_filename_and_path(self):
'-sSEPARATE_DWARF_URL=http://somewhere.com/hosted.wasm'])
self.assertIn(b'somewhere.com/hosted.wasm', read_binary('a.out.wasm'))

@crossplatform
def test_dwarf_system_lib(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you mark this new test a @crossplatform so it runs on mac and windows too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Contributor Author

@kon72 kon72 Dec 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It looks the test is failing on Windows due to path separators (could be related to WebAssembly/binaryen#2781)

From CI output:

DW_AT_name\t("system\\\\lib/emmalloc.c")
DW_AT_comp_dir\t("/emsdk/emscripten")

@sbc100
Should we modify the test to make it pass on Windows? or drop @crossplatform?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to fix if possible. Perhaps if this is a pre-existing issue maybe we don't have to fix it now, but it would be good to fix if possible.

Any idea how system\\\\lib/emmalloc.c could end up like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some digging on my local Windows machine, It seems backslashes in system\\\\lib/emmalloc.c are coming from the source file path we're passing to Clang, and the slash before basename is due to -ffile-reproducible.

Currently, embuilder is passing the source file path in the platform-specific format (with backslashes as separators on Windows):

clang.exe <redacted> -c ..\..\..\system\lib\emmalloc.c

and I get the following DWARF entry

DW_AT_name ("system\\lib/emmalloc.c")

Then when I replace backslashes in the source file path with slashes, the DWARF also have slashes in its file names.

clang.exe <redacted> -c ../../../system/lib/emmalloc.c
DW_AT_name ("system/lib/emmalloc.c")

Therefore, I think the best approach is to apply utils.normalize_path to source file names in the build command.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

That looks right to me.

if config.FROZEN_CACHE:
self.skipTest("test doesn't work with frozen cache")
self.run_process([EMBUILDER, 'build', 'libemmalloc', '--force'])
libc = os.path.join(config.CACHE, 'sysroot', 'lib', 'wasm32-emscripten', 'libemmalloc.a')
self.assertExists(libc)

dwdump = self.run_process(
[LLVM_DWARFDUMP, libc, '-debug-info', '-debug-line', '--recurse-depth=0'],
stdout=PIPE).stdout
# Check that the embedded location of the source file is correct.
self.assertIn('DW_AT_name\t("system/lib/emmalloc.c")', dwdump)
self.assertIn('DW_AT_comp_dir\t("/emsdk/emscripten")', dwdump)

@parameterized({
'O0': (['-O0'],),
'O1': (['-O1'],),
Expand Down
5 changes: 5 additions & 0 deletions tools/system_libs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,9 @@ def generate_ninja(self, build_dir, libname):
cflags = self.get_cflags()
if self.deterministic_paths:
source_dir = utils.path_from_root()
relative_source_dir = os.path.relpath(source_dir, build_dir)
cflags += [f'-ffile-prefix-map={source_dir}=/emsdk/emscripten',
f'-ffile-prefix-map={relative_source_dir}/=',
'-fdebug-compilation-dir=/emsdk/emscripten']
asflags = get_base_cflags(preprocess=False)
input_files = self.get_files()
Expand All @@ -491,7 +493,9 @@ def build_objects(self, build_dir):
cflags = self.get_cflags()
if self.deterministic_paths:
source_dir = utils.path_from_root()
relative_source_dir = os.path.relpath(source_dir, build_dir)
cflags += [f'-ffile-prefix-map={source_dir}=/emsdk/emscripten',
f'-ffile-prefix-map={relative_source_dir}/=',
'-fdebug-compilation-dir=/emsdk/emscripten']
case_insensitive = is_case_insensitive(build_dir)
for src in self.get_files():
Expand Down Expand Up @@ -531,6 +535,7 @@ def build_objects(self, build_dir):
# Use relative paths to reduce the length of the command line.
# This allows to avoid switching to a response file as often.
src = os.path.relpath(src, build_dir)
src = utils.normalize_path(src)
batches.setdefault(tuple(cmd), []).append(src)
objects.add(o)

Expand Down