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

[NativeAOT] Broken COMDAT support in ObjWriter #77491

Open
2 of 4 tasks
filipnavara opened this issue Oct 26, 2022 · 8 comments
Open
2 of 4 tasks

[NativeAOT] Broken COMDAT support in ObjWriter #77491

filipnavara opened this issue Oct 26, 2022 · 8 comments

Comments

@filipnavara
Copy link
Member

filipnavara commented Oct 26, 2022

There are multiple bugs in ObjWriter emission of COMDAT sections:

  • For COFF there's always one .debug$S section which is incorrect. For methods emitted in COMDAT sections there should a new associated COMDAT .debug$S section for each (eg. EmitCVDebugFunctionInfo should have similar section handling code to EmitWinFrameInfo).
  • For COFF the xdata section with UNWIND_INFO structures is not emitted as associated COMDAT. This mostly works due to unique symbol prefixes but it's not ideal.
  • For ELF incorrect relocation types are produced between sections (RELPTR32 is not expressed as PLT relocation). This results in error in ObjWriter if one tries to use ILCompiler with --multifile switch.
  • For MachO the support is completely missing. It can likely be implemented with the S_COALESCE feature which serves the same purpose.
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 26, 2022
@jkotas
Copy link
Member

jkotas commented Oct 26, 2022

@filipnavara Thank you for finding and reporting these issues. Do you plan to submit fixes for these issues?

@filipnavara
Copy link
Member Author

filipnavara commented Oct 26, 2022

This particular set of issues is very low priority for me. I don't think the --multifile scenario is properly tested at the moment and I doubt it works outside of very narrow set of conditions. I do plan to submit a batch of ObjWriter fixes which may help a bit as a side effect.

Depending on feedback to #77178 I may reprioritise. Most of the bugs were found when comparing my C# ObjWriter to the output of the LLVM ObjWriter, or when trying to use LLVM ObjWriter with non-default options.

@MichalStrehovsky
Copy link
Member

I don't think the --multifile scenario is properly tested at the moment

We run it in the CI with a smoke test so that it doesn't completely bitrot but it hasn't been a big priority:

https://github.com/dotnet/runtime/tree/main/src/tests/nativeaot/SmokeTests/MultiModule

IIRC it never worked outside Windows. These ObjWriter issues were likely the reason.

Multifile needs a bit more work to productize and it's unclear we need it right now (it speeds up compilation, but suppresses a lot of whole program optimizations, which makes it only suitable for debug builds and we have a JIT-based solution for debug builds/inner loop in .NET).

@MichalStrehovsky MichalStrehovsky added this to the Future milestone Oct 27, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Oct 27, 2022
@filipnavara
Copy link
Member Author

We run it in the CI with a smoke test so that it doesn't completely bitrot

The tests run in Release mode. I am not even sure whether they are compiled with debug symbols. If the -g switch was not used, then at least the debug symbol and relocation issues would be untested.

@filipnavara
Copy link
Member Author

The description of the Mach-O mechanism in the original post was inaccurate. Apparently the magic sauce are weak def symbols, which used to have the requirement for S_COALESCED section. LLVM stopped emitting the special S_COALESCED sections though since newer linker doesn't need them. Weak def symbols can be present anywhere.

/*
 * The N_WEAK_DEF bit of the n_desc field indicates to the static and dynamic
 * linkers that the symbol definition is weak, allowing a non-weak symbol to
 * also be used which causes the weak definition to be discared.  Currently this
 * is only supported for symbols in coalesed sections.
 */
#define N_WEAK_DEF	0x0080 /* coalesed symbol is a weak definition */

@MichalStrehovsky
Copy link
Member

@filipnavara off the top of your head, do you know if we ported these bugs over to the managed object writer?

@filipnavara
Copy link
Member Author

The COFF bugs were fixed. Mach-O support is still missing. ELF would need to be revisited and rechecked.

@MichalStrehovsky
Copy link
Member

Thanks! Will keep this open then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants