-
Notifications
You must be signed in to change notification settings - Fork 10
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
Bugfix/3169 stack fix #3182
Bugfix/3169 stack fix #3182
Conversation
… stack implementation; the HAVE_RUNTIME_THREAD_STACK_TRACE constant is always True, removed associated build options and internal logic
… stack implementation; the HAVE_RUNTIME_THREAD_STACK_TRACE constant is always True, removed associated build options and internal logic
allows the jni module to make exceptions on demand (no cost with no exception) like:
|
…ded language info to exceptions, updates tests and docs
now a native Java example:
and Qore from Java from Qore:
|
@sejvlond now this is ready for review - call stack info is only created when necessary - when there is a request for the call stack or an exception is raised. Modules providing language integration (ex Also a The default exception and warning handlers share the same code for formatting the output - it's in one place now, so if we want to rework it to make it more easily machine parsable, we can do so. Looking forward to your c++ comments :) |
include/qore/ExceptionSink.h
Outdated
DLLEXPORT const QoreProgramLocation& get() const { | ||
return *loc; | ||
} | ||
|
||
//! returns the file name | ||
DLLEXPORT const char* getFile() const { | ||
return file_str.c_str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why returning const char *
instead of string
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
include/qore/ExceptionSink.h
Outdated
DLLLOCAL QoreStackLocation& operator=(const QoreStackLocation&) = delete; | ||
|
||
//! no move assignment operator | ||
DLLLOCAL QoreStackLocation& operator=(QoreStackLocation&&) = delete; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for deleting copy and move =operators? it looks that the member pointer is not owned and copies are just shallow copies..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
DLLLOCAL QoreStackLocation& operator=(QoreStackLocation&&) = delete; | ||
|
||
//! called when pushed on the stack to set the next location | ||
DLLLOCAL void setNext(const QoreStackLocation* next) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this methods assumes that the QoreStackLocation next
will be valid after setNext as long as this class instance exists. But since QoreStackLocation is a simple object holding just a pointer we can copy it and we don't have to be careful that the QoreStackLocation next
will be there as long as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the problem or how to solve it - can you please give me more info?
next
is a pointer that must be valid while the current object is valid, because it's the current last stack location. It will only disappear after the current item has been deleted. If there is a better way to do this, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
who is the owner of the pointer and when it gets deleted? because the API of this method takes a const* - readonly pointer somewhere - there are no guaranties that the pointer will be valid and still the method saves it into a member and accesses it later. So either the QoreStackLocation
class is going to be owner of it, then I would do setNext(std::unique_ptr<const QoreStackLocation> next)
or is not, and then it must be internally copied.
otherwise someone can do
QoreStackLocation loc1;
{
QoreStackLocation loc2;
loc1.setNext(&loc2);
}
loc1.next().smth - FAIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same for the member right, it's just a pointer without knowing who owns it, I would go with unique or shared_ptr probably...?
Or as I said in a different comment, maybe the StackLocation should hold only info about the location itself and the linked list should be done by std::list - single responsibility for the object.
//! Stack location element abstract class | ||
/** @since %Qore 0.9 | ||
*/ | ||
class QoreStackLocation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class looks like an implementation of a linked list, maybe we can use std::list for it and have QoreStackLocation as a template class for it holding only info about the current Program, statement etc, not holding the list directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it's implemented like this with a virtual function to return the next stack location is so that language modules (ex jni
) can put a single placeholder on the stack, and then if and when an exception is raised, the object can populate all the stack entries in the language stack, the number of which entries will only be known at runtime in case of an exception.
The function returning the next ptr looks like this:
const QoreStackLocation* QoreJniStackLocationHelper::getNext() const {
checkInit();
assert(current < size);
return (++current < size) ? this : stack_next;
}
So basically the "substack" of Java stack entries is created on demand if the stack is actually traversed due to an exception.
class QoreInternalCallStackLocationHelper : public QoreInternalCallStackLocationHelperBase { | ||
public: | ||
DLLLOCAL QoreInternalCallStackLocationHelper(const QoreProgramLocation& loc, const char* call, | ||
qore_call_t call_type) : loc(loc), call(call), call_type(call_type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call
const char *
is stored into a member but who is the owner and how it is forced that the pointer is going to be valid as long as this instance lives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
class QoreProgramStackLocationHelper { | ||
public: | ||
DLLLOCAL QoreProgramStackLocationHelper(QoreStackLocation* stack_loc, const AbstractStatement*& current_stmt, | ||
QoreProgram*& current_pgm) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calling the ctor of QoreProgramStackLocationHelper
changes the current_stmt
and current_pgm
? I don't get it why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it retrieves the current program and statement so they can be returned by the the QoreStackLocation
using that object in its stack position - this functionality is used by the debugger.
No description provided.