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

C++ EH: initial front end work #5342

Merged
merged 1 commit into from Jan 18, 2016
Merged

C++ EH: initial front end work #5342

merged 1 commit into from Jan 18, 2016

Conversation

WalterBright
Copy link
Member

This should do it for the front end.

sc.func.flags |= flags;
if (flags == (FUNCFLAGcppExceptions | FUNCFLAGdExceptions))
{
error("cannot mix catching D and C++ exceptions in the same function");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been pointed out to you there that it is a gratuitous restriction. You can e.g. just look at the language code for the exception to distinguish between them. Could you at least add a "yet" to the error message? That restriction unnecessarily convolutes the design from an user point of view.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've read it, and still don't understand why you need two personality functions. There is no technical difficulty in using the same for both. Take a look at Ada if you don't believe me.

http://www.adacore.com/adaanswers/gems/gem-114-ada-and-c-exceptions/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid the Ada example says nothing whatsoever about what is going on under the hood, and so is not enlightening.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can e.g. just look at the language code for the exception to distinguish between them.

Of course, but that's only half the problem. The other half is the pointer in the TType table - a TypeInfo pointer needs to be distinguished from a C++ RTTI pointer. Elie suggests wrapping the C++ info in a TypeInfo. That may work for dmd, I have not investigated it completely yet. If it doesn't work, or is impractical, the other way to distinguish what kind of pointers are in TType is to use a different personality function that sets a flag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In C++ the TType value is a pointer to some rtti data. While I can accept being wrong, I always considered it to be:

  • Found candidate (D/C++/other), get_ttype_entry_for it.
  • If generic exception we are propagating is domestic (D exception class), proceed as usual.
  • If both TType entry language and exception class hint at being C++, get the underlying exception type (cast exception to __cxa_exception*) and do pointer comparison with the TType value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could set aside some time to try it out in gdc and give feedback.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointer comparison of the TType entries doesn't work, because you can catch the base class of the thrown type. This is true for both D and C++.

The exception class is NOT helpful in determining what types are caught. The only thing that tells what is in the TType is the personality function itself.

We could always invent our own LSDA format, but I'd rather not go that far.

@WalterBright
Copy link
Member Author

Added code to emit the call to __cxa_end_catch()

@WalterBright
Copy link
Member Author

These are the ways to do it:

  1. examine what TType is pointing to and guess by the contents which one it is
  2. have TType point to another struct which wraps TypeInfo and RTTI and has a field saying which it is
  3. wrap RTTI with TypeInfo
  4. have a different personality function for functions which catch C++ types, which sets a flag saying that the TType's are RTTI pointers

(4) is far and away the most straightforward, but it does come with the cost of not mixing RTTI and TypeInfo in the same function, which does not seem an egregious limitation to me. Elie's proposal (3) has some issues with where is the RTTI info defined - I don't want to wire RTTI info into dmd, I think it is more robust to regard them as opaque pointers.

@IgorStepanov
Copy link
Contributor

@WalterBright, where can I read, how do you plan to do that? AFAIR, if you want to catch FooException, you should call __cxa_current_exception_type, get std::type_info and determine, does the catched object can be casted to FooException. Otherwise, you need to rethrow exception. The most strange moment for me, how do you plan to use non-standardized std::type_info to determining of clas hierarchy. Or you plan to catch only exactly matching types?

@ibuclaw
Copy link
Member

ibuclaw commented Jan 10, 2016

I don't think that helps for in-flight routines.

@@ -26,6 +26,7 @@ struct Target
extern (C++) static __gshared int realpad; // 'padding' added to the CPU real size to bring it up to realsize
extern (C++) static __gshared int realalignsize; // alignment for reals
extern (C++) static __gshared bool reverseCppOverloads; // with dmc and cl, overloaded functions are grouped and in reverse order
extern (C++) static __gshared bool cppExceptions; // set if catching C++ exceptions is supported
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also target.h. :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@dnadlinger
Copy link
Member

@WalterBright: If you want to go for the "simple" design (as the others pointed out, I'm not sure if it is actually simpler since you need to at least mangle and reference the type info anyway), couldn't you just use any of the usual tricks to steal a bit from the type pointer to encode the C++/D distinction?

@Syniurge
Copy link
Contributor

The other half is the pointer in the TType table - a TypeInfo pointer needs to be distinguished from a C++ RTTI pointer. Elie suggests wrapping the C++ info in a TypeInfo.

  1. examine what TType is pointing to and guess by the contents which one it is
  2. have TType point to another struct which wraps TypeInfo and RTTI and has a field saying which it is
  3. wrap RTTI with TypeInfo

Actually what I suggested is more like (1). To avoid meddling with TypeInfo while making the distinction possible I simply wrapped the type_info pointers inside a small D class:

class __cpp_type_info_ptr {
    type_info *p;
}

Then cast() is used to determine whether the TType pointer is a TypeInfo or a __cpp_type_info_ptr.

If you plan to use TypeInfo_Class to check if a C++ catch type handles the caught exception (whereas Calypso does it with type_info::__do_catch()) you could add a TypeInfo member to that small class.

@WalterBright
Copy link
Member Author

you need to at least mangle and reference the type info anyway),

Shouldn't be a problem since we already do C++ mangling.

couldn't you just use any of the usual tricks to steal a bit from the type pointer to encode the C++/D distinction?

I prefer to avoid such, as they play hell with things that try to analyze memory usage (like GCs).

@WalterBright
Copy link
Member Author

@Syniurge thanks for helping with this. It is indeed a good idea you have, better than my wrapping idea and better than David's pointer bit idea. It does have a cost of an extra dynamic cast for each catch clause checked, though that is unlikely to be much of a slowdown considering all the other code the personality routine executes.

I plan on using __do_catch regardless of which method is used.

We still need to exhort people to stop using D exceptions as a normal flow-of-control mechanism. I'm looking at you, druntime demangle.d :-)

@dnadlinger
Copy link
Member

you need to at least mangle and reference the type info anyway),

Shouldn't be a problem since we already do C++ mangling.

My point exactly.

I prefer to avoid such, as they play hell with things that try to analyze memory usage (like GCs).

In this case the table is static, refers to global symbols, and wouldn't even be decoded by any of the usual tools/GCs anyway. In any case, I'm not trying to suggest that you should go for the quick hack (especially since you might need to get a bit creative with relocations). I'm just arguing that there is no reason to try and push in a half-baked implementation that is more complex than necessary because it contains gratuitous restrictions.

@WalterBright
Copy link
Member Author

I thought it was enabled by default now?

??

@dnadlinger
Copy link
Member

@WalterBright: @Syniurge is Ellie from the forums (and of Calypso fame), and his implementation is what we have been suggesting from the beginning.

@dnadlinger
Copy link
Member

@WalterBright:

I thought it was enabled by default now?

??

That's a line comment on the explicit -dwarfeh flag being passed to the test cases. If I understand correctly, it is enabled by default now that #5324 has been merged.

@WalterBright
Copy link
Member Author

@klickverbot Actually, the alternate personality entry point is the simplest of all the proposals here. It would also be the fastest and smallest method - set a flag and then call a common _personality_impl(). The restrictions it imposes are hardly onerous, in fact, I'd be skeptical of code in one function that tried to catch both D and C++ exceptions as being poorly encapsulated and abstracted. Yes, I know who @Syniurge is.

dwarfeh is there to "turn on" dwarf eh for platforms it is currently not implemented for. It is there purely for making it easy on me to develop/debug dwarf eh on other platforms without having to constantly rebuild the compiler. I found it convenient to add to the failing test case in order to get it to generate the same fail message consistently on all platforms.

@IgorStepanov
Copy link
Contributor

Ok, I understood. We should use type_info::__do_catch to exception upcasting.

BTW, the similar issue: can we perform dynamic casting for extern(C++) classes via the same method?

//C++
class Base1
{
  ...
}

class Base2
{
  ...
}

class Derived : public Base1, public Base2
{
}


//D
extern(C++) interface Base1
{
   ...
}

extern(C++) interface Base2
{
   ...
}

extern(C++) interface Derived : Base1, Base2
{
   ...
}

extern(C++)  Derived  getDerived();

void test()
{
    auto derived = getDerived();

    Base2 base2 = derived; 
   /*
     compiler should generate:
     void* __tmp = cast(void*)derived;
     bool ret = typeid(Derived).__do_upcast(typeid(Base2), &__tmp);
     base2 = cast(Base2)__tmp;
   */
    auto derived_2 = cast(Derived)base2;
    /*
      compiler should generate:
      void* __tmp = cast(void*)base2;
      if(!typeid(Base2).__do_upcast(typeid(Derived), &__tmp))
           __tmp = null;
      derived_2 = cast(Base2)__tmp;
    */
}

Of course, we should able to get c++ type_info object for an extern(C++) interface declaration.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 11, 2016

@IgorStepanov - that is unrelated here I think. I'd suggest raising a bug report if you think that's a problem.

@MartinNowak
Copy link
Member

OT

We still need to exhort people to stop using D exceptions as a normal flow-of-control mechanism. I'm looking at you, druntime demangle.d :-)

We have to rewrite the whole module b/c it must become @nogc to be used for stacktraces.

@WalterBright
Copy link
Member Author

I finished the initial implementation in the compiler. Decided to go with Elie's approach, though still one cannot mix catching C++ and D exceptions in the same try-catch, because I didn't want the generated code to require the existence of C++ runtime libraries unless C++ exceptions were actually in the code.

Once this passes the autotester, it's on to the Phobos support!

final const(char)* mangle_typeinfo(Dsymbol s)
{
buf.writestring("_ZTI");
source_name(s);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be cpp_mangle_name(s, false).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at the very least be able to handle namespaces in the same way.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 16, 2016

FYI, I've got it working without needing __cpp_type_info_ptr. See here: ibuclaw/GDC@088ceae#diff-bdb3d683e5179a08b5cd05ce4a5b8b4cR692

@IgorStepanov
Copy link
Contributor

FYI, I've got it working without needing __cpp_type_info_ptr.

Is it correct to compare two type_info pointers instead if __do_catch call?
ibuclaw/GDC@088ceae#diff-bdb3d683e5179a08b5cd05ce4a5b8b4cR699
Does your implementation requires exactly matching of the exception type and catch parameter type? Or I missed something?

@ibuclaw
Copy link
Member

ibuclaw commented Jan 16, 2016

Is it correct to compare two type_info pointers instead if __do_catch call?

We don't generate C++ typeinfo, how on earth do you expect this to work?

@IgorStepanov
Copy link
Contributor

We don't generate C++ typeinfo, how on earth do you expect this to work?

I expect that we can catch only those C++ types which are defined in C++ code.
Thus we don't need to generate type_info object, all we need is get pointer to the C++ type_info through correct mangling.

Now we need to define in D extern(C++) type_info interface, which declares __do_catch method.

extern(C++, std) interface type_info 
{
public:
   ~this (); //dummy for the vtbl

  // the internal interface
  bool __is_pointer_p () const;
  bool __is_function_p () const;
  bool __do_catch (const type_info __thr_type, void **__thr_obj,
                         unsigned __outer) const;

  /*
    bool __do_upcast (const __cxxabiv1::__class_type_info *__target,
                            void **__obj_ptr) const;*/
#endif
};

This is the most strange moment, where we need to define a correct D binding for std::type_info

After that we should call __do_catch instead of pointer comparsion:

std.type_info catch_ti = type_entry;
void* e_ptr = thrown_ptr;
std.type_info exception_ti = cxa.exceptionType;

bool is_catched = catch_ti.__do_catch(exception_ti, &e_ptr, 1);
if (is_catched)
{
   // We have successfully catched the exception. e_ptr contains the correct pointer to it.
}
else
{
  //Fail, we need try another catch statement.
}

@ibuclaw
Copy link
Member

ibuclaw commented Jan 16, 2016

Now we need to define in D extern(C++) type_info interface, which declares __do_catch method.

I don't want druntime to depend on libstdc++. That is quite out the question.

@WalterBright
Copy link
Member Author

We don't generate C++ typeinfo, how on earth do you expect this to work?

extern reference to the typeinfo generated by C++. If you're throwing C++ exceptions, it is reasonable to expect linking to libstdc++, and the C++ compiler will generate the typeinfo.

I don't want druntime to depend on libstdc++.

Should be doable since the only references are to std::type_info, and they are via virtual functions.

@IgorStepanov
Copy link
Contributor

I don't want druntime to depend on libstdc++. That is quite out the question.

There is no dependence in my code. Interface is only interface, __do_catch is a virtual function and compiler doesn't emit extern symbol reference here.
Thus you may use druntime with this code without libstdc++ if you don't need a C++ stuff.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 16, 2016

I gave it a go an it seems to work without quarrel. However:

extern(C++, std)
{ 
  interface type_info
  { 
    void dummy(); // ??? <--- Otherwise gets wrong offset.
    void dtor();
    bool is_pointer();
    bool is_function();
    bool do_catch(type_info, void**, uint);
    bool do_upcast(void*, void**);
  }
}

Direct comparison becomes a call to:

static bool
get_adjusted_ptr(type_info catch_type, type_info throw_type, void **thrown_ptr_p)
{
  void *thrown_ptr = *thrown_ptr_p;

  // Pointer types need to adjust the actual pointer, not
  // the pointer to pointer that is the exception object.
  if (throw_type.is_pointer())
    thrown_ptr = *cast(void **) thrown_ptr;

  if (catch_type.do_catch(throw_type, &thrown_ptr, 1))
    {
      *thrown_ptr_p = thrown_ptr;
      return true;
    }

  return false;
}

@IgorStepanov
Copy link
Contributor

// ??? <--- Otherwise gets wrong offset.

It may be "deleting destructor" (D0) and "complete object destructor" (D1). AFAIK, C++ compilers generates more then one destructors for the one class.

if (throw_type.is_pointer())

Do you allow to catch pointers? And maybe you should do this casting after do_catch call?

static bool
get_adjusted_ptr(type_info catch_type, type_info throw_type, void **thrown_ptr_p)
{
  void *thrown_ptr = *thrown_ptr_p;

  if (catch_type.do_catch(throw_type, &thrown_ptr, 1))
    {
      *thrown_ptr_p = thrown_ptr;
      return true;
    }


  // Pointer types need to adjust the actual pointer, not
  // the pointer to pointer that is the exception object.
  // This also has the effect of passing pointer types
  // "by value" through the __cxa_begin_catch return value.
  if (throw_type.is_pointer())
      thrown_ptr = *cast(void **) thrown_ptr;


  return false;
}

@WalterBright
Copy link
Member Author

I've got it working now. Will clean it up and prepare a PR.

@WalterBright
Copy link
Member Author

This will fail until dlang/druntime#1470 is pulled.

version (linux)
{
version (X86_64)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indents?

@andralex
Copy link
Member

Auto-merge toggled on

andralex added a commit that referenced this pull request Jan 18, 2016
C++ EH: initial front end work
@andralex andralex merged commit 605be4c into dlang:master Jan 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants