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

write/coff: Merge all .rdata$.refptr. sections #474

Closed
bjorn3 opened this issue Oct 8, 2022 · 9 comments · Fixed by #475
Closed

write/coff: Merge all .rdata$.refptr. sections #474

bjorn3 opened this issue Oct 8, 2022 · 9 comments · Fixed by #475

Comments

@bjorn3
Copy link
Contributor

bjorn3 commented Oct 8, 2022

It seems that in https://github.com/bjorn3/rustc_codegen_cranelift/issues/1249#issuecomment-1272290580 the windows linker spends most of the time (almost 75%) churning through the names of all these sections.

@philipc
Copy link
Contributor

philipc commented Oct 10, 2022

I think the current behaviour is copying what mingw does. Can you give me an example object file generated by the llvm backend?

@afonso360
Copy link
Contributor

afonso360 commented Oct 10, 2022

I took a random lib (libbevy_ecs) that appears in the dependencies of bevy and here's what I got.

llvm-objdump -h libbevy_ecs_llvm.rlib

llvm-objdump -h libbevy_ecs_cgclif.rlib

We print fewer sections 2497 vs llvm which spits out 30376. However, llvm produces them all with the same name, so I wonder if MSVC is able to deal with that better and possibly merge them all before processing somehow.

Here's a link to a repo with both libraries, github does not allow me to attach them to this comment.

Its worth noting that I only see this behaviour with MSVC's link.exe, if i try to link with rust-lld.exe everything works as normal, so it really appears to be a MSVC only issue.

I'm going to try to minimize our section names and see if the issue goes away.

@afonso360
Copy link
Contributor

afonso360 commented Oct 10, 2022

Yep, that seems to have done the trick! I applied this commit to duplicate our section names and it now links in a reasonable time.

Would you like me to open a PR for that, or should we deal with this in a different manner?

@philipc
Copy link
Contributor

philipc commented Oct 10, 2022

llvm isn't emitting refptr symbols at all in that library. The reason it has a large number of sections is because llvm is using a separate section for each function and data symbol, while cgclif has a single section for each of .text, .rdata, and .data.

I don't want to apply a commit without first having a complete understanding:

  • Is there any way to get llvm to emit refptr symbols, and if so, what sections does it create then?
  • Does cgclif really need to emit refptr symbols, or should we provide a way for cgclif to avoid emitting them?
  • If we duplicate section names, does it matter what they are called? Is there a reason you chose .rdata$.refptr?

@afonso360
Copy link
Contributor

afonso360 commented Oct 10, 2022

I don't really know how to answer the first two questions, but maybe @bjorn3 can help?

If we duplicate section names, does it matter what they are called? Is there a reason you chose .rdata$.refptr?

Not really, I just tried with .thisisatest and it also works alright. I chose .rdata$.refptr because it was what was there before (without the trailing .).

@bjorn3
Copy link
Contributor Author

bjorn3 commented Oct 10, 2022

The reason it has a large number of sections is because llvm is using a separate section for each function and data symbol

AFAIK rustc disables -ffunction-section on Windows to avoid hitting the section count limit of COFF.

Does cgclif really need to emit refptr symbols, or should we provide a way for cgclif to avoid emitting them?

When is_pic is enabled, Cranelift emits GotRelative relocations when accessing data. Object lowers this to .rdata$.refptr.* for COFF object files. Without is_pic enabled it isn't possible to link the object files into dylibs afaik and we don't know in advance if a crate will end up being linked into an executable or a dylib.

@philipc
Copy link
Contributor

philipc commented Oct 10, 2022

AFAIK rustc disables -ffunction-section on Windows to avoid hitting the section count limit of COFF.

Why does libbevy_ecs_llvm.rlib have a lot of .text sections then?

When is_pic is enabled, Cranelift emits GotRelative relocations when accessing data. Object lowers this to .rdata$.refptr.* for COFF object files. Without is_pic enabled it isn't possible to link the object files into dylibs afaik and we don't know in advance if a crate will end up being linked into an executable or a dylib.

libbevy_ecs_llvm.rlib doesn't have these refptr stubs. Is there a difference in what knowledge is available to llvm versus cranelift?

@bjorn3
Copy link
Contributor Author

bjorn3 commented Oct 10, 2022

Why does libbevy_ecs_llvm.rlib have a lot of .text sections then?

Just checked the rustc source and it seems it is only disabled on MinGW. Based on some discussion I found elsewhere it seems that MSVC and MinGW both have a trick now to avoid the section limit using COMDAT, but both in incompatible ways. It is still disabled on MinGW due to a binutils bug.

libbevy_ecs_llvm.rlib doesn't have these refptr stubs. Is there a difference in what knowledge is available to llvm versus cranelift?

I think there is some difference in how PE and ELF implement dylibs that Cranelift doesn't know about. I'm not familiar enough with PE to tell what though.

@philipc
Copy link
Contributor

philipc commented Oct 11, 2022

Based on https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#grouped-sections-object-only, I think it is okay to rename the sections to .rdata$.refptr, so please create a PR with that change. I still think that Cranelift is using GotRelative more than it should, but that is outside the scope of this crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants