Skip to content

Generalize passes that require RTTI #2127

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

Conversation

peterschrammel
Copy link
Member

A small extension to languaget allows us to make the following passes language-independent:

  • remove_virtual_methods
  • remove_exceptions
  • remove_instanceof

The caveat is that (ignoring CPP which needs to be reimplemented) there is currently no other language that would use these facilities.

Alternatively, we could choose to move these passes to java_bytecode, with the (at the moment not much worrying) risk that incompatible infrastructures for similar functionalities will be re-implemented for future languages.

@smowton
Copy link
Contributor

smowton commented Apr 27, 2018

Seems sensible -- two possible changes:

  • Instantiating a languaget to answer the question "what is your class hierarchy root?", and doing that once per instanceof expression, seems rather heavyweight. How about introducing some language_infot or similar, which is stateless, and which is instantiated once and cached to answer questions like this?
  • Why are you removing the remove_exceptions special case for AssertionError?

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I can't claim serious java_bytecode knowledge.

#include <goto-programs/remove_function_pointers.h>
#include <goto-programs/remove_virtual_functions.h>
#include <goto-programs/remove_instanceof.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit pick: lexicographic ordering

#include <goto-programs/remove_function_pointers.h>
#include <goto-programs/remove_virtual_functions.h>
#include <goto-programs/remove_instanceof.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit pick: lexicographic ordering

#include <goto-programs/remove_virtual_functions.h>
#include <goto-programs/remove_instanceof.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit pick: lexicographic ordering

id2string(uncaught_exceptions_domaint::get_exception_type(exc.type())).
find("java.lang.AssertionError")!=std::string::npos;
if(!is_assertion_error)
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be a test to cover this?

@peterschrammel peterschrammel force-pushed the lang-indep-virtual-instanceof branch from cb61d28 to b3ba2cc Compare April 28, 2018 20:33
@peterschrammel
Copy link
Member Author

@smowton, yes, that's indeed very, very inefficient. We cannot transparently cache the language in the global languages list, but we could provide a caching wrapper that could be used where necessary.

@smowton
Copy link
Contributor

smowton commented Apr 30, 2018

@peterschrammel in fact on closer inspection we sort of have this mechanism already -- the languge_entryt used in langapi/mode.cpp does something very similar, avoiding needing to instantiate languaget in order to answer "what file extensions are associated with this language," or "what mode is associated with this language?" How about extending language_entryt, or having registering languages supply a language_entryt instead of just a factory?

@thk123
Copy link
Contributor

thk123 commented May 1, 2018

Am holding off on review this due to above comment suggests a potential re-work - let me know if I should review as is.

@martin-cs
Copy link
Collaborator

+1, what @thk123 said. Am happy to review if this is live again.

FWIW Ada will need something like remove virtual functions and remove exceptions and I know that @andreast271 is working on C++ and so might care about having space for that.

@peterschrammel
Copy link
Member Author

I'm going to revive this.
It needs a bit of rework since we are moving from a 'framework-provides-dispatch' to a 'driver-provides-dispatch' mechanism for language-specific features.

@thomasspriggs
Copy link
Contributor

This needs rebasing due to a number of merge conflicts and running out of date CI jobs. @peterschrammel are you still planning to revive this? If this is work which is no longer planned then we should close it out in order to clean up the number of open PRs.

@peterschrammel
Copy link
Member Author

It's probably easier to reimplement these few lines than to rebase them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants