[PATCH 1/3] makedep: Align PE sections so they can be mmapped.
This should help linux perf tool match the binary files on disk with the code regions in memory. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- This is my old perf patch series rebased and using the recent CROSSDEBUG variable to get split debug info. I'm assuming that the only use cases are PDB split debug info or DWARF split debug info into ELF files for perf, as I don't know any other, but if having DWARF debug in PE files is actually useful for something then these patches are incorrect. I could also see that some CROSSDEBUG checks look for "split" prefix in makedep, and some other set the variable to dwarf. It's not clear to me what this variable is supposed to be set to (other than "pdb"). Does "dwarf" mean separate file as well? tools/makedep.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/makedep.c b/tools/makedep.c index 536d2263e35f..250250382fa8 100644 --- a/tools/makedep.c +++ b/tools/makedep.c @@ -3317,6 +3317,7 @@ static void output_module( struct makefile *make ) if (debug_file) output_filename( strmake( "-Wl,--debug-file,%s", debug_file )); output_filenames( all_libs ); output_filename( make->is_cross ? "$(CROSSLDFLAGS)" : "$(LDFLAGS)" ); + output_filename( make->is_cross ? "-Wl,--file-alignment,4096" : "" ); output( "\n" ); if (make->unixobj_files.count) -- 2.26.1
Linux perf isn't able to parse debug link from PE files either, but it looks for similarly named files in relative a .debug directory. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- tools/makedep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/makedep.c b/tools/makedep.c index 250250382fa8..b550534fd789 100644 --- a/tools/makedep.c +++ b/tools/makedep.c @@ -2356,7 +2356,7 @@ static const char *get_debug_file( struct makefile *make, const char *name ) const char *debug_file = NULL; if (!make->is_cross || !crossdebug) return NULL; if (!strcmp( crossdebug, "pdb" )) debug_file = strmake( "%s.pdb", get_base_name( name )); - else if(!strncmp( crossdebug, "split", 5 )) debug_file = strmake( "%s.debug", name ); + else if(!strncmp( crossdebug, "split", 5 )) debug_file = strmake( ".debug/%s", name ); if (debug_file) strarray_add( &make->debug_files, debug_file ); return debug_file; } -- 2.26.1
Hi Rémi, On 25.05.2020 16:32, Rémi Bernon wrote:
Linux perf isn't able to parse debug link from PE files either, but it looks for similarly named files in relative a .debug directory.
Did you look at how hard it would be to add minimal PE support to perf itself? To replace this patch, it would need to locate GNU debug link in PE file. Its format is identical to ELF files, so I'd hope that it shouldn't be too hard. It's a similar situation for debug info itself. Thanks, Jacek
On 5/25/20 5:15 PM, Jacek Caban wrote:
Hi Rémi,
On 25.05.2020 16:32, Rémi Bernon wrote:
Linux perf isn't able to parse debug link from PE files either, but it looks for similarly named files in relative a .debug directory.
Did you look at how hard it would be to add minimal PE support to perf itself? To replace this patch, it would need to locate GNU debug link in PE file. Its format is identical to ELF files, so I'd hope that it shouldn't be too hard. It's a similar situation for debug info itself.
Thanks,
Jacek
I didn't look very deep but I think although it already uses libbfd for some things, it directly uses libelf to look for debug info and symbols (and thus only supports ELF files for that purpose). It may be possible to change that and make it use libbfd for everything and support PE files but it seemed to be a lot more work than generating the files in the right format (also, the .debug folder is apparently some kind of standard method as well so it wasn't completely perf-specific). -- Rémi Bernon <rbernon(a)codeweavers.com>
When not using PDB, as Linux perf is only able to read debug info from ELF files. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- tools/winegcc/winegcc.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/winegcc/winegcc.c b/tools/winegcc/winegcc.c index b59d9c2c81b7..22e902073595 100644 --- a/tools/winegcc/winegcc.c +++ b/tools/winegcc/winegcc.c @@ -1412,9 +1412,13 @@ static void build(struct options* opts) if (opts->debug_file && !strendswith(opts->debug_file, ".pdb")) { + char const *bfd_format = (opts->target_cpu == CPU_x86_64) ? "-Oelf64-x86-64" : + (opts->target_cpu == CPU_x86) ? "-Oelf32-i386" : + NULL; strarray *tool, *objcopy = build_tool_name(opts, TOOL_OBJCOPY); tool = strarray_dup(objcopy); + if (bfd_format) strarray_add(tool, bfd_format); strarray_add(tool, "--only-keep-debug"); strarray_add(tool, output_path); strarray_add(tool, opts->debug_file); -- 2.26.1
On 25.05.2020 16:32, Rémi Bernon wrote:
I could also see that some CROSSDEBUG checks look for "split" prefix in makedep, and some other set the variable to dwarf. It's not clear to me what this variable is supposed to be set to (other than "pdb"). Does "dwarf" mean separate file as well?
No, "dwarf" means that debug info will be embedded in module PE files, which has always been the default. "dwarf" part of "split-dwarf" is important for configure script to apply dwarf-specific flags. "dwarf," "split-dwarf" and "pdb" are the only configs for which I know a real use, but in theory you could set it to anything else as long as you take care of CROSSCFLAGS yourself. "stabs" and "split-stabs" should be in theory achievable this way (but I didn't try). Jacek
On 5/25/20 5:26 PM, Jacek Caban wrote:
On 25.05.2020 16:32, Rémi Bernon wrote:
I could also see that some CROSSDEBUG checks look for "split" prefix in makedep, and some other set the variable to dwarf. It's not clear to me what this variable is supposed to be set to (other than "pdb"). Does "dwarf" mean separate file as well?
No, "dwarf" means that debug info will be embedded in module PE files, which has always been the default. "dwarf" part of "split-dwarf" is important for configure script to apply dwarf-specific flags. "dwarf," "split-dwarf" and "pdb" are the only configs for which I know a real use, but in theory you could set it to anything else as long as you take care of CROSSCFLAGS yourself. "stabs" and "split-stabs" should be in theory achievable this way (but I didn't try).
Jacek
Alright. So, "split*" means that the debug sections are extracted to a separate file, and, currently, this file is also a PE file, as the default objcopy operation is to use the same output format as the source file. Having the debug section in an ELF file instead in this case shouldn't be an issue right? Gdb and any other tool that were already able to parse DWARF from PE files should be able to parse DWARF from ELF without trouble. Using an ELF container allows perf to read it. I don't think there's anything that requires PE container with DWARF debug info, but maybe there is? Regarding the default name and location of this ELF module, I see that we already implement in dbghelp the same logic as gdb does, which includes looking for .debug/debugfile, where debugfile is the contents of GNU debuglink, so this should work fine as well, while at the same time making perf work OOTB. From a quick test, as long as the .debug folder is also copied alongside the dll in the prefix, both winedbg and winedbg --gdb are happy with the ELF container. So, in the end, although I understand the root issue is some limitation of perf itself, this feels much simpler to fix in Wine with these small tweaks that makes every tool happy, than to try implementing PE support in perf. I think it would require much more work there, and would be way harder to justify if it were to be upstreamed (this is clearly a Wine specific use-case and I'm not sure how eager perf maintainers would be to have huge code changes just for the sake of PE module parsing). -- Rémi Bernon <rbernon(a)codeweavers.com>
On 27.05.2020 13:49, Rémi Bernon wrote:
On 5/25/20 5:26 PM, Jacek Caban wrote:
On 25.05.2020 16:32, Rémi Bernon wrote:
I could also see that some CROSSDEBUG checks look for "split" prefix in makedep, and some other set the variable to dwarf. It's not clear to me what this variable is supposed to be set to (other than "pdb"). Does "dwarf" mean separate file as well?
No, "dwarf" means that debug info will be embedded in module PE files, which has always been the default. "dwarf" part of "split-dwarf" is important for configure script to apply dwarf-specific flags. "dwarf," "split-dwarf" and "pdb" are the only configs for which I know a real use, but in theory you could set it to anything else as long as you take care of CROSSCFLAGS yourself. "stabs" and "split-stabs" should be in theory achievable this way (but I didn't try).
Jacek
Alright. So, "split*" means that the debug sections are extracted to a separate file, and, currently, this file is also a PE file, as the default objcopy operation is to use the same output format as the source file.
Having the debug section in an ELF file instead in this case shouldn't be an issue right? Gdb and any other tool that were already able to parse DWARF from PE files should be able to parse DWARF from ELF without trouble. Using an ELF container allows perf to read it. I don't think there's anything that requires PE container with DWARF debug info, but maybe there is?
Regarding the default name and location of this ELF module, I see that we already implement in dbghelp the same logic as gdb does, which includes looking for .debug/debugfile, where debugfile is the contents of GNU debuglink, so this should work fine as well, while at the same time making perf work OOTB.
From a quick test, as long as the .debug folder is also copied alongside the dll in the prefix, both winedbg and winedbg --gdb are happy with the ELF container.
So, in the end, although I understand the root issue is some limitation of perf itself, this feels much simpler to fix in Wine with these small tweaks that makes every tool happy, than to try implementing PE support in perf. I think it would require much more work there, and would be way harder to justify if it were to be upstreamed (this is clearly a Wine specific use-case and I'm not sure how eager perf maintainers would be to have huge code changes just for the sake of PE module parsing).
Yes, I agree that it could all work, I even made sure that dbghelp.dll can handle it when I was adding support for debug links in PE. I'm not against (at least part) of the patchset. But the thing is that it's quite a lot of perf-specific tweaking. Even if we take those patches, it would still be interesting to be better supported by perf, so why not at least evaluate it first. I took a quick look at perf sources and I'm not convinced that it would be hard. It already has some abstraction, see dso_binary_type enum and its usage. It already has special cases for things like DSO_BINARY_TYPE__JAVA_JIT, so DSO_BINARY_TYPE__PE doesn't seem out of place. The amount of PE understanding that perf would need to find debug info is very minimal: it just needs to be able to enumerate sections. The above is about two other patches. I didn't look at PE alignment fix, but I just expect that it should be fixable upstream. With that in mind, my comments to patches: - PE alignment The patch breaks build with llvm-mingw because lld doesn't support --file-alignment. It's also not exactly free. Note that you could just pass CROSSLDFLAGS=... to configure instead of patching Wine. - .debug directory I don't have a strong opinion here. If it's indeed a standard, maybe we should follow it. GDB documentation mentions both %s.debug and .debug/%s and, not being a fan of hidden directories in build tree, I chose the one that seemed nicer: https://sourceware.org/gdb/current/onlinedocs/gdb/Separate-Debug-Files.html - symbols in ELF file We could do that, creating an exotic mix then building on mac, with PE, Mach-O and ELF files. Another option would be CROSSLDFLAGS=-Wl,--objcopy,-Oelf64-x86-64, but it doesn't seem nice... Thanks, Jacek
On Fri, 29 May 2020, Jacek Caban wrote:
- PE alignment
The patch breaks build with llvm-mingw because lld doesn't support --file-alignment.
FWIW, this should be trivial to add support for in lld's mingw frontend - lld-link underneath it supports the corresponding link.exe style option.
- symbols in ELF file
We could do that, creating an exotic mix then building on mac, with PE, Mach-O and ELF files. Another option would be CROSSLDFLAGS=-Wl,--objcopy,-Oelf64-x86-64, but it doesn't seem nice...
FWIW, llvm-objcopy doesn't support converting to another object file format than the input, at the moment - and _that_ bit is a much larger task to take on. // Martin
On 5/29/20 2:57 PM, Jacek Caban wrote:
On 27.05.2020 13:49, Rémi Bernon wrote:
On 5/25/20 5:26 PM, Jacek Caban wrote:
On 25.05.2020 16:32, Rémi Bernon wrote:
I could also see that some CROSSDEBUG checks look for "split" prefix in makedep, and some other set the variable to dwarf. It's not clear to me what this variable is supposed to be set to (other than "pdb"). Does "dwarf" mean separate file as well?
No, "dwarf" means that debug info will be embedded in module PE files, which has always been the default. "dwarf" part of "split-dwarf" is important for configure script to apply dwarf-specific flags. "dwarf," "split-dwarf" and "pdb" are the only configs for which I know a real use, but in theory you could set it to anything else as long as you take care of CROSSCFLAGS yourself. "stabs" and "split-stabs" should be in theory achievable this way (but I didn't try).
Jacek
Alright. So, "split*" means that the debug sections are extracted to a separate file, and, currently, this file is also a PE file, as the default objcopy operation is to use the same output format as the source file.
Having the debug section in an ELF file instead in this case shouldn't be an issue right? Gdb and any other tool that were already able to parse DWARF from PE files should be able to parse DWARF from ELF without trouble. Using an ELF container allows perf to read it. I don't think there's anything that requires PE container with DWARF debug info, but maybe there is?
Regarding the default name and location of this ELF module, I see that we already implement in dbghelp the same logic as gdb does, which includes looking for .debug/debugfile, where debugfile is the contents of GNU debuglink, so this should work fine as well, while at the same time making perf work OOTB.
From a quick test, as long as the .debug folder is also copied alongside the dll in the prefix, both winedbg and winedbg --gdb are happy with the ELF container.
So, in the end, although I understand the root issue is some limitation of perf itself, this feels much simpler to fix in Wine with these small tweaks that makes every tool happy, than to try implementing PE support in perf. I think it would require much more work there, and would be way harder to justify if it were to be upstreamed (this is clearly a Wine specific use-case and I'm not sure how eager perf maintainers would be to have huge code changes just for the sake of PE module parsing).
Yes, I agree that it could all work, I even made sure that dbghelp.dll can handle it when I was adding support for debug links in PE. I'm not against (at least part) of the patchset. But the thing is that it's quite a lot of perf-specific tweaking. Even if we take those patches, it would still be interesting to be better supported by perf, so why not at least evaluate it first.
Sure it's perf specific, but to be honest I'm not seeing any actual use case for this separate debug file except for perf compatibility. I'm not talking about the PDB here, just the separate PE module with DWARF debug infos. I understand why debug symbols are usually split, for instance by OS distributions, to reduce the size of the packages, but it is usually done already using build wrappers. I'm maybe not aware of all use cases but right now I don't see any use of having the DWARF debug info being split after link for normal wine builds.
I took a quick look at perf sources and I'm not convinced that it would be hard. It already has some abstraction, see dso_binary_type enum and its usage. It already has special cases for things like DSO_BINARY_TYPE__JAVA_JIT, so DSO_BINARY_TYPE__PE doesn't seem out of place. The amount of PE understanding that perf would need to find debug info is very minimal: it just needs to be able to enumerate sections.
Well DSO_BINARY_TYPE__JAVA_JIT is really some ugly hack to make perf a little bit more flexible and let some other tool dump a symbol list with address ranges in a "perfmap" text file at runtime. Other than that the dso_binary_type enumeration is mostly used to tell where to find the debug file, not really the format it's in. The code the assumes ELF format all over the place, even using libelf constants internally. The "perfmap" support is just some plumbing on top of it, parsing the file and inserting ELF-like symbols in the internal list. It's indeed possible to do the same thing for PE files, as implemented in the attached patches, using libbfd to parse them, but it still feels very hacky. Then I may be pessimistic but I also expect it to be a bit hard to justify such a change, as it is -in the other way this time- a wine-specific change in perf. Also, although the basic features seems to work fine (perf annotate relies on external tools for disassembly for instance), it's limited to providing the symbol list and it's not going to be well integrated with the ELF/DWARF part of the code, so it may lack features.
The above is about two other patches. I didn't look at PE alignment fix, but I just expect that it should be fixable upstream. With that in mind, my comments to patches:
I'm not sure to understand, you mean fixable in perf as well? I don't think it is easily fixable there either, perf uses mmap hooks to detect which files are mapped to which memory range, and if the section aren't aligned we copy them instead, which makes it loose track.
- PE alignment
The patch breaks build with llvm-mingw because lld doesn't support --file-alignment. It's also not exactly free. Note that you could just pass CROSSLDFLAGS=... to configure instead of patching Wine.
Sure and all these patches can actually be implemented with some build system wrapper, I just think it'd be nice if perf could handle default wine builds.
- .debug directory
I don't have a strong opinion here. If it's indeed a standard, maybe we should follow it. GDB documentation mentions both %s.debug and .debug/%s and, not being a fan of hidden directories in build tree, I chose the one that seemed nicer:
https://sourceware.org/gdb/current/onlinedocs/gdb/Separate-Debug-Files.html
- symbols in ELF file
We could do that, creating an exotic mix then building on mac, with PE, Mach-O and ELF files. Another option would be CROSSLDFLAGS=-Wl,--objcopy,-Oelf64-x86-64, but it doesn't seem nice...
I don't know how useful ELF or Mach-O format would be on mac, we could leave the PE format there if that's better. But again, I don't really understand the use case for this non-PDB split debug file.
Thanks,
Jacek
Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
On 30.05.2020 21:45, Rémi Bernon wrote:
On 5/29/20 2:57 PM, Jacek Caban wrote:
On 27.05.2020 13:49, Rémi Bernon wrote:
Alright. So, "split*" means that the debug sections are extracted to a separate file, and, currently, this file is also a PE file, as the default objcopy operation is to use the same output format as the source file.
Having the debug section in an ELF file instead in this case shouldn't be an issue right? Gdb and any other tool that were already able to parse DWARF from PE files should be able to parse DWARF from ELF without trouble. Using an ELF container allows perf to read it. I don't think there's anything that requires PE container with DWARF debug info, but maybe there is?
Regarding the default name and location of this ELF module, I see that we already implement in dbghelp the same logic as gdb does, which includes looking for .debug/debugfile, where debugfile is the contents of GNU debuglink, so this should work fine as well, while at the same time making perf work OOTB.
From a quick test, as long as the .debug folder is also copied alongside the dll in the prefix, both winedbg and winedbg --gdb are happy with the ELF container.
So, in the end, although I understand the root issue is some limitation of perf itself, this feels much simpler to fix in Wine with these small tweaks that makes every tool happy, than to try implementing PE support in perf. I think it would require much more work there, and would be way harder to justify if it were to be upstreamed (this is clearly a Wine specific use-case and I'm not sure how eager perf maintainers would be to have huge code changes just for the sake of PE module parsing).
Yes, I agree that it could all work, I even made sure that dbghelp.dll can handle it when I was adding support for debug links in PE. I'm not against (at least part) of the patchset. But the thing is that it's quite a lot of perf-specific tweaking. Even if we take those patches, it would still be interesting to be better supported by perf, so why not at least evaluate it first.
Sure it's perf specific, but to be honest I'm not seeing any actual use case for this separate debug file except for perf compatibility. I'm not talking about the PDB here, just the separate PE module with DWARF debug infos.
For me, it was indeed a side quest when working on PDB files motivated by your use case. I was hoping that we will figure out questionable parts later, which brings us to this thread. But it provides some improvements regardless of perf. For example, it significantly reduces prefix size created by an unstripped build. I was also thinking about using split debug for tests instead of linking them twice.
I understand why debug symbols are usually split, for instance by OS distributions, to reduce the size of the packages, but it is usually done already using build wrappers.
Yeah, and if we could get it supported by perf then whatever distributions do (like using build id), they could do the same to PE files and perf would work.
It's indeed possible to do the same thing for PE files, as implemented in the attached patches, using libbfd to parse them, but it still feels very hacky. Then I may be pessimistic but I also expect it to be a bit hard to justify such a change, as it is -in the other way this time- a wine-specific change in perf.
Also, although the basic features seems to work fine (perf annotate relies on external tools for disassembly for instance), it's limited to providing the symbol list and it's not going to be well integrated with the ELF/DWARF part of the code, so it may lack features.
Great, thanks for looking at it! It already supports more than we can achieve by adjusting Wine, right? I don't know if upstream would be interested in something like this, but I think that it would be worth asking them.
The above is about two other patches. I didn't look at PE alignment fix, but I just expect that it should be fixable upstream. With that in mind, my comments to patches:
I'm not sure to understand, you mean fixable in perf as well? I don't think it is easily fixable there either, perf uses mmap hooks to detect which files are mapped to which memory range, and if the section aren't aligned we copy them instead, which makes it loose track.
Oh, right. Being more friendly for the loader is a valid argument to use it in Wine. I measured build size change by checking 32-bit prefix size (so not exactly measuring build size, but with shared Gecko and Mono installs, it's mostly PEs that are in prefix and I already had it scripted). GCC build size increased from 133MB to 152MB, clang build (where I used CROSSLDFLAGS=-Wl,-Xlink=-filealign:409) from 121MB to 139MB. This is not great, but probably not a deal breaker. This would still need a configure check. Thanks, Jacek
Jacek Caban <jacek(a)codeweavers.com> writes:
Oh, right. Being more friendly for the loader is a valid argument to use it in Wine.
I measured build size change by checking 32-bit prefix size (so not exactly measuring build size, but with shared Gecko and Mono installs, it's mostly PEs that are in prefix and I already had it scripted). GCC build size increased from 133MB to 152MB, clang build (where I used CROSSLDFLAGS=-Wl,-Xlink=-filealign:409) from 121MB to 139MB. This is not great, but probably not a deal breaker. This would still need a configure check.
It should probably be handled in winegcc instead of configure. Being able to mmap files is also a good thing WRT memory usage, so I feel it would be worth doing even if the size increases a bit. -- Alexandre Julliard julliard(a)winehq.org
participants (4)
-
Alexandre Julliard -
Jacek Caban -
Martin Storsjö -
Rémi Bernon