Page MenuHomePhabricator

[clang] [MinGW] Fix libunwind extension
ClosedPublic

Authored by mati865 on Fri, May 15, 4:47 AM.

Diff Detail

Event Timeline

mati865 created this revision.Fri, May 15, 4:47 AM
mati865 changed the repository for this revision from rC Clang to rG LLVM Github Monorepo.Fri, May 15, 5:29 AM
mati865 added a project: Restricted Project.
mati865 added a subscriber: cfe-commits.
mstorsjo added a subscriber: rnk.

So, using -l:libunwind.dll.a is definitely more correct than -l:libunwind.so on this platform, so in that sense this is good.

In my toolchains I don't use this option at all so far, because I have libunwind bundled into libc++.a/libc++.dll.a, but I guess it would be nice to move towards this setup and stop bundling them. But the toolchains can be configured to omit either the shared or the static version of the libraries, and if libunwind only exists in static form, this would fail.

If there's only a shared version, and building with -static-libgcc, I guess it's ok to look for explicitly libunwind.a and fail in that case, but for the default case, I think the normal library resolution logic would be best, i.e. just doing -lunwind.

The ideal version from my point of view would be this:

case ToolChain::UNW_CompilerRT:
  if (LGT == LibGccType::StaticLibGcc)
    CmdArgs.push_back("-l:libunwind.a");
  else if (TC.getTriple().isOSCygMing()) {
    if (LGT == LibGccType::SharedLibGcc)
      CmdArgs.push_back("-l:libunwind.dll.a");
    else
      CmdArgs.push_back("-lunwind"); // Let the linker choose between libunwind.dll.a and libunwind.a depending on what's available, and depending on the -static flag
  } else
    CmdArgs.push_back("-l:libunwind.so");
  break;

That's rather different than the other, non-mingw cases though, but I'm not familiar with the reasoning behind those.

Adding @rnk if he'd happen to have opinions.

mati865 updated this revision to Diff 264292.Fri, May 15, 11:01 AM

Applied review comment and formatted.

mati865 added a comment.EditedTue, May 26, 2:50 AM

@mstorsjo @rnk will be away since June

@mstorsjo @rnk will be away since June

Yeah, I've noticed - but was waiting to see if he'd comment on it before that.

Btw, would it be possible to add some test for this? (Sorry for not mentioning it earlier.) clang/test/Driver/compiler-rt-unwind.c seems to have existing testcases for this, either add more to that one or make a copy of the file and add new mingw specific ones there.

mati865 updated this revision to Diff 267034.Thu, May 28, 2:17 PM

Added test.

mati865 updated this revision to Diff 267036.Thu, May 28, 2:19 PM
mstorsjo accepted this revision.Thu, May 28, 2:34 PM

LGTM now - thanks! What's your preferred git author line for this project? I can probably push it tomorrow.

This revision is now accepted and ready to land.Thu, May 28, 2:34 PM

No worries.
Mateusz Mikuła <mati865@gmail.com>

This revision was automatically updated to reflect the committed changes.