Skip to content

Commit

Permalink
[hphpd] fixed a couple of issues around the debugger
Browse files Browse the repository at this point in the history
Summary:
- found a huge bug with the way how Constants are stored with ClassInfo. Before
the diff, there are thread-local constants got into m_constants of ClassInfos.
That's really bad. Not only these variants are co-accessed by multiple threads
without setStatic() protection, which isn't available to STDIN, STDOUT and
STDERR objects, but also a separate thread can simply sweep them and crash other
threads or new requests almost immediately. I think we're lucky that people
don't get constant's values on production. But this definitely caused debugger
to crash mysteriously.

 - made breakpoint's index monotonically increasing without re-index.

 - added DebuggerDump for special serialization of variables for better display
of variables.

 - breakpoints now matching BaseClass::method() if ThisClass::method() isn't
present.

 - added "exception error" command to catch errors, warning, notices and
fatals.

 - fixed some minor issues with "list" command after "frame" commands.

Test Plan:
manual test

DiffCamp Revision: 151804
Reviewed By: mwilliams
CC: mwilliams, hphp-diffs@lists
Revert Plan:
OK
  • Loading branch information
haiping authored and macvicar committed Sep 2, 2010
1 parent b400dac commit b6f4739
Show file tree
Hide file tree
Showing 25 changed files with 286 additions and 118 deletions.
1 change: 1 addition & 0 deletions doc/debugger.refs
Expand Up @@ -163,6 +163,7 @@ to move down several levels a time.
[e]xception {cls} breaks if class of exception throws
[e]xception breaks if class of exception throws
{ns}::{cls}
[e]xception error breaks on errors, warnings and notices
[e]xception breaks only if url also matches
{above}@{url}

Expand Down
11 changes: 1 addition & 10 deletions src/runtime/base/builtin_functions.cpp
Expand Up @@ -806,16 +806,7 @@ Variant invoke_static_method_bind_mil(CStrRef s,
// debugger and code coverage instrumentation

void throw_exception(CObjRef e) {
if (RuntimeOption::EnableDebugger) {
ThreadInfo *ti = ThreadInfo::s_threadInfo.get();
if (ti->m_reqInjectionData.debugger) {
Eval::InterruptSite site(ti->m_top, e);
Eval::Debugger::InterruptException(site);
if (site.isJumping()) {
return;
}
}
}
if (!Eval::Debugger::InterruptException(e)) return;
throw e;
}

Expand Down
38 changes: 28 additions & 10 deletions src/runtime/base/class_info.cpp
Expand Up @@ -185,20 +185,38 @@ const ClassInfo::ConstantInfo *ClassInfo::FindConstant(const char *name) {
return info;
}

ClassInfo::ConstantInfo::ConstantInfo() : callbacks(NULL), deferred(true) {
}

Variant ClassInfo::ConstantInfo::getValue() const {
if (deferred) {
if (callbacks == NULL) {
return get_constant(name);
}
return callbacks->os_constant(name);
}
return value;
}

void ClassInfo::ConstantInfo::setValue(CVarRef v) {
value = v;
deferred = false;
}

Array ClassInfo::GetConstants() {
if (!s_loaded) Load();
Array res;
Array dyn;
{
const ConstantMap &scm = s_systemFuncs->getConstants();
for (ConstantMap::const_iterator it = scm.begin(); it != scm.end(); ++it) {
res.set(it->first, it->second->value);
res.set(it->first, it->second->getValue());
}
}
{
const ConstantMap &ucm = s_userFuncs->getConstants();
for (ConstantMap::const_iterator it = ucm.begin(); it != ucm.end(); ++it) {
res.set(it->first, it->second->value);
res.set(it->first, it->second->getValue());
}
}
if (s_hook) {
Expand Down Expand Up @@ -496,7 +514,7 @@ bool ClassInfo::checkAccess(ClassInfo *defClass,
bool staticCall,
bool hasObject) const {
ASSERT(defClass && methodInfo);
if (!strcasecmp(this->m_name, defClass->m_name)) {
if (!strcasecmp(m_name, defClass->m_name)) {
if (methodInfo->attribute & ClassInfo::IsStatic) return true;
return hasObject;
}
Expand Down Expand Up @@ -650,8 +668,9 @@ ClassInfoUnique::ClassInfoUnique(const char **&p) {
staticVariable->valueLen));
VariableUnserializer vu(in);
try {
staticVariable->value = vu.unserialize();
staticVariable->value.setStatic();
Variant v = vu.unserialize();
v.setStatic();
staticVariable->setValue(v);
} catch (Exception &e) {
ASSERT(false);
}
Expand Down Expand Up @@ -686,22 +705,21 @@ ClassInfoUnique::ClassInfoUnique(const char **&p) {
istringstream in(std::string(constant->valueText, constant->valueLen));
VariableUnserializer vu(in);
try {
constant->value = vu.unserialize();
constant->value.setStatic();
Variant v = vu.unserialize();
v.setStatic();
constant->setValue(v);
} catch (Exception &e) {
ASSERT(false);
}
} else if (m_name) {
if (!(m_attribute & IsVolatile) && !(m_attribute & IsLazyInit)) {
const ObjectStaticCallbacks *cwo = get_object_static_callbacks(m_name);
if (cwo) {
constant->value = cwo->os_constant(constant->name);
constant->callbacks = cwo;
} else {
ASSERT(false);
}
}
} else {
constant->value = get_constant(constant->name);
}

ASSERT(m_constants.find(constant->name) == m_constants.end());
Expand Down
12 changes: 11 additions & 1 deletion src/runtime/base/class_info.h
Expand Up @@ -69,10 +69,20 @@ class ClassInfo {
HasOptFunction = (1 << 23) // x
};

struct ConstantInfo {
class ConstantInfo {
public:
ConstantInfo();

const char *name;
unsigned int valueLen;
const char *valueText;
const ObjectStaticCallbacks *callbacks;

Variant getValue() const;
void setValue(CVarRef value);

private:
bool deferred;
Variant value;
};

Expand Down
3 changes: 3 additions & 0 deletions src/runtime/base/execution_context.cpp
Expand Up @@ -29,6 +29,7 @@
#include <runtime/base/runtime_option.h>
#include <runtime/base/debug/backtrace.h>
#include <runtime/base/server/server_stats.h>
#include <runtime/eval/debugger/debugger.h>
#include <util/logger.h>
#include <util/process.h>
#include <util/text_color.h>
Expand Down Expand Up @@ -558,11 +559,13 @@ void ExecutionContext::handleError(const std::string &msg,
handled = callUserErrorHandler(ee, errnum, false);
}
if (mode == AlwaysThrow || (mode == ThrowIfUnhandled && !handled)) {
if (!Eval::Debugger::InterruptException(String(msg))) return;
throw FatalErrorException(msg.c_str());
}
if (!handled &&
(RuntimeOption::NoSilencer ||
(getErrorReportingLevel() & errnum) != 0)) {
if (!Eval::Debugger::InterruptException(String(msg))) return;
Logger::Log(prefix.c_str(), ee);
}
}
Expand Down
18 changes: 14 additions & 4 deletions src/runtime/base/variable_serializer.cpp
Expand Up @@ -30,9 +30,10 @@ using namespace std;
namespace HPHP {
///////////////////////////////////////////////////////////////////////////////

VariableSerializer::VariableSerializer(Type type, int option /* = 0 */)
VariableSerializer::VariableSerializer(Type type, int option /* = 0 */,
int maxRecur /* = 3 */)
: m_type(type), m_option(option), m_buf(NULL), m_indent(0),
m_valueCount(0), m_referenced(false), m_refCount(1), m_maxCount(3),
m_valueCount(0), m_referenced(false), m_refCount(1), m_maxCount(maxRecur),
m_outputLimit(0) {
}

Expand Down Expand Up @@ -78,6 +79,7 @@ void VariableSerializer::write(bool v) {
break;
case VarExport:
case JSON:
case DebuggerDump:
m_buf->append(v ? "true" : "false");
break;
case VarDump:
Expand All @@ -103,6 +105,7 @@ void VariableSerializer::write(int64 v) {
case PrintR:
case VarExport:
case JSON:
case DebuggerDump:
m_buf->append(v);
break;
case VarDump:
Expand Down Expand Up @@ -149,6 +152,7 @@ void VariableSerializer::write(double v) {
break;
case VarExport:
case PrintR:
case DebuggerDump:
{
char *buf;
if (v == 0.0) v = 0.0; // so to avoid "-0" output
Expand Down Expand Up @@ -202,6 +206,7 @@ void VariableSerializer::write(const char *v, int len /* = -1 */,
m_buf->append(v, len);
break;
}
case DebuggerDump:
case VarExport: {
if (len < 0) len = strlen(v);
m_buf->append('\'');
Expand Down Expand Up @@ -347,6 +352,7 @@ void VariableSerializer::writeNull() {
m_buf->append("N;");
break;
case JSON:
case DebuggerDump:
m_buf->append("null");
break;
default:
Expand All @@ -372,6 +378,7 @@ void VariableSerializer::writeOverflow(void* ptr, bool isObject /* = false */) {
throw NestingLevelTooDeepException();
case VarDump:
case DebugDump:
case DebuggerDump:
indent();
m_buf->append("*RECURSION*\n");
break;
Expand Down Expand Up @@ -511,6 +518,7 @@ void VariableSerializer::writeArrayHeader(const ArrayData *arr, int size) {
}
break;
case JSON:
case DebuggerDump:
if (info.is_vector) {
m_buf->append("[");
} else {
Expand Down Expand Up @@ -658,6 +666,7 @@ void VariableSerializer::writeArrayKey(const ArrayData *arr, Variant key) {
}
break;
case JSON:
case DebuggerDump:
if (!info.first_element) {
m_buf->append(",");
}
Expand Down Expand Up @@ -727,6 +736,7 @@ void VariableSerializer::writeArrayFooter(const ArrayData *arr) {
m_buf->append('}');
break;
case JSON:
case DebuggerDump:
if (info.is_vector) {
m_buf->append("]");
} else {
Expand Down Expand Up @@ -773,6 +783,8 @@ bool VariableSerializer::incNestedLevel(void *ptr,
case PrintR:
case VarDump:
case DebugDump:
case JSON:
case DebuggerDump:
return ++m_counts[ptr] >= m_maxCount;
case Serialize:
case APCSerialize:
Expand All @@ -787,8 +799,6 @@ bool VariableSerializer::incNestedLevel(void *ptr,
return ct >= (m_maxCount - 1);
}
break;
case JSON:
return ++m_counts[ptr] >= m_maxCount;
default:
ASSERT(false);
break;
Expand Down
3 changes: 2 additions & 1 deletion src/runtime/base/variable_serializer.h
Expand Up @@ -41,6 +41,7 @@ class VariableSerializer {
VarExport,
VarDump,
DebugDump,
DebuggerDump,
Serialize,
JSON,
APCSerialize,
Expand All @@ -49,7 +50,7 @@ class VariableSerializer {
/**
* Constructor.
*/
VariableSerializer(Type type, int option = 0);
VariableSerializer(Type type, int option = 0, int maxRecur = 3);

/**
* Top level entry function called by f_ functions.
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/eval/ast/class_statement.cpp
Expand Up @@ -498,8 +498,8 @@ void ClassStatement::getInfo(ClassInfoEvaled &info) const {
it != m_constants.end(); ++it) {
ClassInfo::ConstantInfo *c = new ClassInfo::ConstantInfo;
c->name = it->first.c_str();
c->value = it->second->eval(dv);
String sv = c->value.toString();
c->setValue(it->second->eval(dv));
String sv = c->getValue().toString();
char* buf = new char[sv.size()+1];
memcpy(buf, sv.data(), sv.size()+1);
c->valueLen = sv.size();
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/eval/ast/function_statement.cpp
Expand Up @@ -372,7 +372,7 @@ void FunctionStatement::getInfo(ClassInfo::MethodInfo &info) const {
ci->valueLen = 12;
ci->valueText = "unsupported";
if (it->second) {
ci->value = it->second->eval(env);
ci->setValue(it->second->eval(env));
}
info.staticVariables.push_back(ci);
}
Expand Down

0 comments on commit b6f4739

Please sign in to comment.