[PATCH 1/3] makedep: Align PE sections so they can be mmapped.

[Jacek Caban]

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