[PATCH] D122258: [MC][RFC] Omit DWARF unwind info if compact unwind is present for all archs

[Lang Hames via Phabricator via cfe-commits]

lhames added a comment.

> I'm currently doing that via MCContext::getGenDwarfForAssembly...

This isn't my wheelhouse, but at first glance GenDwarfForAssembly looks like it might control all Dwarf sections (including debug ones). If that's the case you might need to introduce something eh-frame specific.

> Previously, omitting unnecessary DWARF unwinds was only done for
> watchOS, but I've heard that it might be possible to do this for all
> archs.

Sounds like this was for bincompat with older unwinders when compact-unwind was first released. I think we can turn this off by default now.

> I believe the root cause is that libunwind is unable to load compact
> unwind dynamically. (I believe this limitation applies not just to MCJIT but to
> ORC and the regular interpreter as well -- @lhames, would you be able to
> confirm?)

That's right. Adding dynamic registration for compact unwind is pretty high on my todo list. I'm hoping to get to it within the next month or so. We should still have a way to force eh-frames though.



================
Comment at: clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp:48
 
+// FIXME
 TEST(InterpreterTest, CatchException) {
----------------
I guess this is just a placeholder? The final commit should include more detail.


================
Comment at: llvm/lib/ExecutionEngine/RuntimeDyld/RTDyldMemoryManager.cpp:131-132
+                                           size_t Size) {
+  if (Size == 0)
+    return;
   registerEHFramesInProcess(Addr, Size);
----------------
Is this needed? With your change, registerEHFramesInProcess should be a no-op if `Size == 0`.


================
Comment at: llvm/tools/llc/llc.cpp:697-700
+    auto &Ctx = MMIWP->getMMI().getContext();
+    Ctx.setGenDwarfForAssembly(Target->Options.ForceDwarfFrameSection);
     const_cast<TargetLoweringObjectFile *>(LLVMTM.getObjFileLowering())
+        ->Initialize(Ctx, *Target);
----------------
This option hand-off feels very manual. I wonder if we should add a `propagateTargetOptionsToMC` function, but maybe the set of options that it would apply to is small enough not to bother for now?

I would add something like this to `LLVMTargetMachine::addPassesToEmitMC` too, for the the case where the caller passes in a null `MCContext*`. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122258/new/

https://reviews.llvm.org/D122258