This is an archive of the discontinued LLVM Phabricator instance.

Toolchain: Normalize dwarf, sjlj and seh eh
ClosedPublic

Authored by martell on Nov 5 2017, 11:08 PM.

Details

Summary

adds -fseh-exceptions and -fdwarf-exceptions flags

clang will check if the user has specified an exception model flag,
in the absense of specifying the exception model clang will then check
the driver default and append the model flag for that target to cc1

clang cc1 assumes dwarf is the default if none is passed
and -fno-exceptions has a higher priority then specifying the model

move SEH macro definitions out of Targets into InitPreprocessor
behind the -fseh-exceptions flag

move ARM_DWARF_EH macrodefinitions out of verious targets and into
InitPreprocessor behind the -fdwarf-exceptions flag and arm|thumb check

remove unused USESEHExceptions from the MinGW Driver
fold USESjLjExceptions into a new GetExceptionModel function that
gives the toolchain classes more flexibility with eh models.

if a toolchain does not specify the exception model make a best guess based
what llvm does for that triple with the assembler much like cc1as.

Diff Detail

Repository
rL LLVM

Event Timeline

martell created this revision.Nov 5 2017, 11:08 PM
martell edited the summary of this revision. (Show Details)Nov 5 2017, 11:12 PM
martell edited the summary of this revision. (Show Details)
mati865 added a subscriber: mati865.Nov 6 2017, 3:34 AM
martell updated this revision to Diff 121832.Nov 6 2017, 6:40 PM

Add command line documentation references

@mstorsjo I think this should help with issues like having to set the default for ARM to DWARF.
If you can set it at run time then it should be a lot easier to just set that in the clang wrapper scripts.

Long term I would like the MinGW Driver to load a different libunwind depending on the exception model but that discussion might still be a little off.
Any thoughts?

mstorsjo edited edge metadata.Nov 6 2017, 11:27 PM

@mstorsjo I think this should help with issues like having to set the default for ARM to DWARF.
If you can set it at run time then it should be a lot easier to just set that in the clang wrapper scripts.

Yes, I think this probably can be useful.

As for the ARM patch (D39532), parts of it are necessary in any case since the ARM/COFF backend doesn't support outputting dwarf yet. Since there's no other option there yet, setting it to dwarf until something else is implemented probably should be fine though.

The main place where this patch really can provide value though, is for the libcxxabi/libunwind based x86_64 builds. Since libcxxabi doesn't support SEH yet (although I'm planning on at least having a look at how much work it'd be to implement it, but I'm afraid it can be a pretty big task), but clang for x86_64 mingw in general does support SEH (and that's what is used for the normal mingw setups) we shouldn't change the default there.

I haven't tried this patch in that setup yet but will do soon.

Long term I would like the MinGW Driver to load a different libunwind depending on the exception model but that discussion might still be a little off.

I don't really see what would be necessary there; a matching unwinding library should be provided in whatever C++ and compiler runtime library you use, so that's mostly an issue for whoever sets up the environment to build libraries in the right configurations matching it as a whole.

@mstorsjo I think this should help with issues like having to set the default for ARM to DWARF.

Btw, in addition to making setting __SEH__ conditional, it should also instead set __ARM_DWARF_EH__ on ARM if dwarf is chosen there (see D39533).

In D39673, @martell wrote:

Long term I would like the MinGW Driver to load a different libunwind depending on the exception model but that discussion might still be a little off.

I don't really see what would be necessary there; a matching unwinding library should be provided in whatever C++ and compiler runtime library you use, so that's mostly an issue for whoever sets up the environment to build libraries in the right configurations matching it as a whole.

I mostly expect this to be useful where environment has builds all 3 versions of the libunwind. where the library is named differently based on the eh model.
e.g. libunwind-sjlj.a libunwind-seh.a and libunwind-dwarf.a
With the above exception flags the version of libunwind with the matching model should be automatically linked, I imagine it to be stored in a similar locations as the compiler-rt libs.

I mostly expect this to be useful where environment has builds all 3 versions of the libunwind. where the library is named differently based on the eh model.
e.g. libunwind-sjlj.a libunwind-seh.a and libunwind-dwarf.a
With the above exception flags the version of libunwind with the matching model should be automatically linked, I imagine it to be stored in a similar locations as the compiler-rt libs.

Ah, I see. I'm a little hesitant about such an environment, because code built with each exception handling model is essentially a different ABI, and having them all in the same setup feels pretty prone to errors. Like, you need to have built your libcxx and libcxxabi with matching exception handling models as well, so then you need three copies of them as well.

rnk accepted this revision.Nov 14 2017, 12:45 PM

Looks good, sorry for the delay!

include/clang/Basic/LangOptions.def
129

"Structured exception handling" is very ambiguous. I would say something like "SEH .xdata exception handling" or something to clarify.

This revision is now accepted and ready to land.Nov 14 2017, 12:45 PM
mstorsjo added inline comments.Nov 15 2017, 10:44 AM
lib/Frontend/InitPreprocessor.cpp
684

Please define __ARM_DWARF_EH__ if dwarf is enabled on arm, see D39533, as I commented earlier. (Sorry, I forgot about it and thought it was fixed already.)

D39533 was committed now, so before committing, make sure that the __ARM_DWARF_EH__ (that was added in that commit) only gets set while dwarf is enabled (which now is the default for mingw/arm but not msvc/arm).

martell updated this revision to Diff 123486.Nov 18 2017, 3:24 PM

updated to handle dwarf exceptions on arm.

martell requested review of this revision.Nov 18 2017, 3:34 PM
martell edited edge metadata.

Thanks for the review @rnk . I addressed the comment nit you had.
@mstorsjo I updated this to also handle thumb on arm.

When doing that I noticed there is something really strange about the existing macro defines. I assume they should only be defined when exceptions is enabled.
This is by default in c++ mode of with -fexceptions in c mode.
I moved the defines within a check of exceptions being enabled to address that.

Apple seem to do their own thing for watchOS by just force enabling it for that target only.
Some input there would help a lot.

Also if you think we should define the macros even when exceptions are disabled could you explain why?

mstorsjo added inline comments.Nov 19 2017, 4:16 AM
lib/Frontend/InitPreprocessor.cpp
685

Won't this start setting this define also on platforms where ARM EHABI is the default? (I.e. all ELF platforms except netbsd)?

martell added inline comments.Nov 19 2017, 8:18 PM
lib/Frontend/InitPreprocessor.cpp
685

It will set it on any arm / thumb platform where exceptions are enabled and the current model is dwarf.
This includes netbsd. The issue we have is that apple set this unconditionally for their watch platform.

I'm wondering if we should put this behind a !apple guard or just add an if windows or netbsd guard.

Or is it okay to just generally have this macro on all platforms where dwarf is exception model and the target is arm?

When doing that I noticed there is something really strange about the existing macro defines. I assume they should only be defined when exceptions is enabled.
This is by default in c++ mode of with -fexceptions in c mode.
I moved the defines within a check of exceptions being enabled to address that.

I'm not sure if this is the right thing to do. Since the exception handling model more or less also defines what ABI the code conforms to, I can see it being useful to know what exception handling mode is intended to be used, even if compiling plain C code without exceptions enabled. E.g. when building libunwind, some of the C sources there have ifdefs that check for __USING_SJLJ_EXCEPTIONS__ and/or __ARM_DWARF_EH__. With this change, one has to manually start specifying it when building libunwind, to match whatever the default and/or manually chosen exception handling model is.

mstorsjo added inline comments.Nov 20 2017, 12:08 AM
lib/Frontend/InitPreprocessor.cpp
685

No, it will set it on any arm/thumb platform where one hasn't indicated preference for any other model. And there are more models than just dwarf/seh/sjlj - there's also the ARM EHABI model. (According to MC/MCTargetOptions.h in llvm, there aren't any more than this currently though.)

Prior to this patch, clang -x c++ -target armv7-linux-gnueabihf -E -dM - < /dev/null will not include __ARM_DWARF_EH__ - while it will after this patch. This will certainly break libunwind for linux on arm. (And prior to this patch, clang -x c -target armv7-netbsd -E -dM - < /dev/null did include it, but now it's only included when exceptions are enabled - I think the old behaviour is preferrable wrt that as well.)

So, similarly how we on x86_64 have a default that enables SEH unless something else has been specified, we need to prefer ARM EHABI on everything on ELF except netbsd. I guess you could fold in the apple defaults into that as well?

martell updated this revision to Diff 123758.Nov 21 2017, 3:45 AM

updated to HEAD.
added a NOT msvc test

I'm not sure if this is the right thing to do. Since the exception handling model more or less also defines what ABI the code conforms to, I can see it being useful to know what exception handling mode is intended to be used, even if compiling plain C code without exceptions enabled. E.g. when building libunwind, some of the C sources there have ifdefs that check for __USING_SJLJ_EXCEPTIONS__ and/or __ARM_DWARF_EH__. With this change, one has to manually start specifying it when building libunwind, to match whatever the default and/or manually chosen exception handling model is.

It seems reasonable to me that one would specify -fexceptions when building c sources to get the macro for the exceptions, libunwind included, but yes right now there are c sources in libunwind that this would break and probably other libraries that have c sources that check unconditionally. This is not an issue for c++ as -fexceptions is enabled by default.

This is a little beyond the scope of what I am trying to achieve in this patch though and should be in a different differential after libunwind adds -fexceptions when building it's c sources, if we go that route.
For now I have update this patch to HEAD and reverted back to using the model macros defines independent of -fexceptions as you suggested.

martell added a comment.EditedNov 21 2017, 4:27 AM

Just as a note there is still a lot to be desired here. I do not particularly like the UseSEHExceptions function default and the actual macro definition guards should be based on the current ExceptionModel because we set that in lib/CodeGen/BackendUtil.cpp. This way we do not need to have some silly default of x64 && windows for UseSEHExceptions and can rely on the llvm backend defaults but override within the driver we want like the apple targets do for sjlj. This does for now keep the current functionality while giving us a flag to override which is the goal of this patch.

rnk added a comment.Nov 21 2017, 9:17 AM

Just as a note there is still a lot to be desired here. I do not particularly like the UseSEHExceptions function default and the actual macro definition guards should be based on the current ExceptionModel because we set that in lib/CodeGen/BackendUtil.cpp. This way we do not need to have some silly default of x64 && windows for UseSEHExceptions and can rely on the llvm backend defaults but override within the driver we want like the apple targets do for sjlj. This does for now keep the current functionality while giving us a flag to override which is the goal of this patch.

We have to know the EH model before pre-processing, and that shouldn't rely on LLVM TargetOptions. We can probably reuse the llvm::ExceptionHandling enum instead of these various overlapping booleans, if that's the direction you want to go. However, I don't see how we can get away from the clang toolchain knowing the default EH model for each target.

No objection from me about committing this now, although I have some minor comments (that possibly can be taken care of without reposting and re-reviewing). Still ok with @rnk?

lib/Frontend/InitPreprocessor.cpp
686

I guess you could add an || LangOpts.DWARFExceptions here as well? That would allow you to manually enable dwarf on arm on linux as well. (I did that manually while testing some bits in libunwind.)

Ideally I'd prefer to have a UseDWARFExceptions just like there already is UseSEHExceptions and UseSjLjExceptions in the driver, so that it'd explcitly pass -fdwarf-exceptions here in the cases where it's known to be the default. But at least this form shouldn't break anything right now.

rnk accepted this revision.Nov 21 2017, 1:41 PM

Yep, looks good

This revision is now accepted and ready to land.Nov 21 2017, 1:41 PM
martell updated this revision to Diff 123883.Nov 22 2017, 1:03 AM
martell edited the summary of this revision. (Show Details)

fold USESjLjExceptions into GetExceptionModel.
do a best effort to check the llvm default exception model for the triple if the toolchain does not specify.
(mostly based on the cc1_as code)

martell added a comment.EditedNov 22 2017, 1:24 AM
In D39673#931861, @rnk wrote:

We have to know the EH model before pre-processing, and that shouldn't rely on LLVM TargetOptions. We can probably reuse the llvm::ExceptionHandling enum instead of these various overlapping booleans, if that's the direction you want to go. However, I don't see how we can get away from the clang toolchain knowing the default EH model for each target.

I ended up folding USESjLjExceptions into a GetExceptionModel function that returns the llvm::ExceptionHandling.
Reid you are right that clang needed to know some targets beforehand, this is especially obvious for the various apple devices. watch and co.

relying on LLVM TargetOptions is much better for platforms like windows
rather then checking T.isOSWindows() && T.getArch() == llvm::Triple::x86_64 to decide on enabling SEH I opted to do a best effort attempt to check what the default is much like how cc1as does.
the difference being if it fails we just specify the default as none giving us the following pattern.

  1. set eh based no user flags passed into clang
  2. if no flags passed check the toolchain defaults,
  3. if no toolchain default then check llvm,
  4. if we can not determine if it is a supported triple default to None.

I also addressed Martin's concern in the folding by adding | LangOpts.DWARFExceptions and no longer have to specifically check for mingw or netbsd.

Sorry for the noise on this, I just want to make sure we get the best functionality on this while ensuring it is future proof for other model.

If you still have strong objections on 3. after seeing this it is pretty trivial to just return None from GetExceptionModel by default and then always rely on the Toolchains to know the models. (though I will have to do some windows specific checks in the ToolChain::GetExceptionModel to ensure seh is the default)

mstorsjo accepted this revision.Nov 22 2017, 12:29 PM

Looks really good to me now! @rnk?

rnk requested changes to this revision.Nov 28 2017, 12:09 PM
rnk added inline comments.
lib/Driver/ToolChain.cpp
450–458 ↗(On Diff #123883)

I think this is problematic because Clang is supposed to be able to generate LLVM IR for targets that are not registered. With this change, we have silent behavior difference between clang with LLVM backend and clang without LLVM backends.

Building clang without a single backend is pretty silly, but our test suite frequently generates IR for targets that are not compiled in. Some of the tests you've written will probably fail on buildbots configured this way.

This revision now requires changes to proceed.Nov 28 2017, 12:09 PM
martell updated this revision to Diff 124692.Nov 28 2017, 9:11 PM
martell edited edge metadata.

address reid's comment
update to HEAD

mstorsjo added inline comments.Nov 28 2017, 9:16 PM
lib/Driver/ToolChain.cpp
458 ↗(On Diff #124692)

I'd suggest braces around the outer if statement.

But is there any point to this default fallback here at all? Dwarf isn't right for x86 for MSVC, and we set the right defaults for all the MinGW cases in that driver in any case. So isn't it enough to just return None always here, or possibly WinEH for all windows cases?

martell added inline comments.Nov 28 2017, 9:53 PM
lib/Driver/ToolChain.cpp
458 ↗(On Diff #124692)

Won't need braces with the change I am about to make.

The previous check was getArch() == llvm::Triple::x86_64 then use seh.
This didn't take into account arm or aarch64 so I flipped this.

I read the wrong llvm class for llvm::Triple::x86 X86MCAsmInfoGNUCOFF
It should have been X86MCAsmInfoMicrosoft I looked at.

if (Triple.getArch() == Triple::x86_64) {
  PrivateGlobalPrefix = ".L";
  PrivateLabelPrefix = ".L";
  CodePointerSize = 8;
  WinEHEncodingType = WinEH::EncodingType::Itanium;
} else {
  // 32-bit X86 doesn't use CFI, so this isn't a real encoding type. It's just
  // a place holder that the Windows EHStreamer looks for to suppress CFI
  // output. In particular, usesWindowsCFI() returns false.
  WinEHEncodingType = WinEH::EncodingType::X86;
}

ExceptionsType = ExceptionHandling::WinEH;

I think None in the case of x86 is the behavior that was present previously so I am going to revert from dwarf to none.

https://www.google.com/patents/US5628016
That borland patent has long since expired now and seh is the default in llvm for x86 so we can do seh on x86 for clang but I will leave that to a different patch.

martell updated this revision to Diff 124695.Nov 28 2017, 10:11 PM
  • disregard my last comment, lets go with seh on all windows targets.
mstorsjo accepted this revision.Nov 28 2017, 10:23 PM

landing this as it was previous approved by reid in this state and again re checked by martin.

This revision was automatically updated to reflect the committed changes.
mstorsjo added inline comments.Nov 28 2017, 10:52 PM
cfe/trunk/lib/Driver/ToolChain.cpp
457 ↗(On Diff #124696)

It looks like this broke some buildbot after all - see e.g. http://lab.llvm.org:8011/builders/sanitizer-windows/builds/20371.

Perhaps it's best to just have the default here be None even for Windows, and just let it be overridden for MinGW? I don't think we did define __SEH__ at all when targeting MSVC before either, so it should be fine to just leave it unset and not override anything from the backend defaults here.

mstorsjo added inline comments.Nov 28 2017, 10:58 PM
cfe/trunk/lib/Driver/ToolChains/FreeBSD.cpp
368 ↗(On Diff #124696)

This seems to need a fallthrough comment to build without warnings in some configurations, see e.g. http://lab.llvm.org:8011/builders/ubuntu-gcc7.1-werror/builds/3329/steps/build-unified-tree/logs/stdio

@mstorsjo yup, I added a comment on the commit about the failure here rL319294
I have already fixed both issues.
Will re apply shortly.

martell added a comment.EditedNov 28 2017, 11:07 PM

I also want to note a small addition armeb and thumbeb for NetBSD.
They were missed in the previous version.
Just waiting for tests to pass now.