[llvm-commits] [llvm] r49006 - in /llvm/trunk/lib: CodeGen/DwarfWriter.cpp CodeGen/LLVMTargetMachine.cpp CodeGen/SelectionDAG/SelectionDAGISel.cpp Target/PowerPC/PPCAsmPrinter.cpp Target/PowerPC/PPCRegisterInfo.cpp Target/X86/X86AsmPrinter.cpp Target/X86/X86RegisterInfo.cpp Transforms/Utils/LowerInvoke.cpp

[Duncan Sands]

Hi Dale,

> Emit exception handling info for functions which are
> not marked nounwind,

that doesn't sound right.  As I had already explained to you
in a private email, the dwarf info has two parts related to
exception handling: the eh table, and frame moves info.  If
a function is nounwind then there is no need to generate the
frame moves info.  If a function has no invokes then there
is no need to generate the eh table.  These are independent.
A nounwind function can have invokes (indeed this is quite
common after optimization - see prune-eh), I see that Anton
sent you an example.  For such a function you should generate
an EH table but no frame moves.
If a nounwind function has no invokes then you can just not
generate dwarf info at all (I'm ignoring debug intrinsics
here, which in general require dwarf info).
And if a not-nounwind function has no invokes then you don't
have to generate an EH table.  I think this is already the
case (hopefully Anton will correct me if I'm wrong): we only
generate an EH table if there is a personality function, and
the only way to have a personality function is if there are
invokes (it is also possible to have invokes without a personality,
but since we don't know how to codegen those you can just
ignore this).

So I think all you need to do is this: make generation of
frame moves optional.  Generate frame moves for a function
if and only if it is not-nounwind (or has debug intrinsics).
If the function doesn't require frame moves or an EH table
then don't generate a dwarf frame for it at all.

> or for all functions when -enable-eh 
> is set, provided the target supports Dwarf EH.

Presumably this is for debugging purposes?  If so, how about
making this a hidden option and renaming it to -debug-eh ?
Otherwise people are going to think they need this for eh
support.  Which they don't, right?  (If I understand you
correctly, you've basically turned on eh support
unconditionally).

> llvm-gcc generates nounwind in the right places; other FEs
> will need to do so also.  Given such a FE, -enable-eh should
> no longer be needed.

> +  /// shouldEmit - Per-function flag to indicate if EH information should
> +  /// be emitted.
>    bool shouldEmit;

This is too crude.  I think you need to distinguish between frame moves
(= locating the values of variables) and the EH table (matching exceptions
to actions).  So you need two variables here.

> +    if ((ExceptionHandling || !MF->getFunction()->doesNotThrow()) &&
>          TAI->doesSupportExceptionHandling()) {
>        shouldEmit = true;

This looks badly wrong: you just broke nounwind functions that contain invokes.

> -  if (!ExceptionHandling)
> -    PM.add(createLowerInvokePass(getTargetLowering()));
> +  PM.add(createLowerInvokePass(getTargetLowering()));

Does this mean that you just lowered all exception handling to setjmp/longjmp?
Hopefully not, but I vaguely recall from the last time I looked at this stuff
that it is surprisingly easy to make a mistake here.

> -  if (!ExceptionHandling)
> -    PM.add(createLowerInvokePass(getTargetLowering()));
> +  PM.add(createLowerInvokePass(getTargetLowering()));

Likewise.

> +  // Figure out whether we need to generate EH info.  Currently we do this for
> +  // all functions not marked no-unwind, or if requested via -enable-eh.
> +  needsExceptionHandling = ExceptionHandling || !Fn.doesNotThrow();

This is wrong, see above.  There's already a test somewhere that only outputs
the EH table if there is a personality function.

>    case Intrinsic::eh_exception: {
> -    if (ExceptionHandling) {
> +    if (FuncInfo.needsExceptionHandling) {

This is backwards.  If a function has invokes then it requires an EH table.
The only way to get the eh_exception intrinsic is to have an invoke.  Thus if
you see eh_exception then the function has an invoke so for sure you want to
lower it (unless eh has been globally turned off, which is what the previous
test was for).  There should be no test here.  You should really just
substitute "true" for ExceptionHandling everywhere...

> -    if (ExceptionHandling && MMI) {
> +    if (FuncInfo.needsExceptionHandling && MMI) {

Likewise for all the other examples of this.

> -  if (LandingPad && ExceptionHandling && MMI) {
> +  if (LandingPad && FuncInfo.needsExceptionHandling && MMI) {
>      // Insert a label before the invoke call to mark the try range.  This can be

Likewise: you're only getting here if you have an invoke, so no test
is needed (see below also).

>  bool LowerInvoke::runOnFunction(Function &F) {
> +  // If we will be generating exception info, don't do anything here.
> +  if ((ExceptionHandling || !F.doesNotThrow()) &&
> +      TLI &&
> +      TLI->getTargetMachine().getTargetAsmInfo()->
> +                              doesSupportExceptionHandling())
> +    return false;

The !F.doesNotThrow() test is bogus.  Also, what is ExceptionHandling doing
here?  Shouldn't the test be: do this if -enable-correct-eh-support was
specified and the target does not support exception handling?

Ciao,

Duncan.