[PATCH] Backtrace for android and linux
Zack Rusin
zack at kde.org
Fri Apr 5 14:22:03 PDT 2013
On Thu, Apr 4, 2013 at 8:02 AM, Eugene Velesevich <evel at ispras.ru> wrote:
> Hello,
Hi, on top of what was said, here's a few comments about the code:
> if (required->find(call->no) != required->end()) {
> + if(!call->bt.empty()) {
Please follow the style of the code around. There should be a space
between if and the parenthesis, here and in many more instances.
> + char* bt = new char [call->bt.length()];
"char *bt" here and also in many more instances
and please try to use the full variable names as much as possible,
i.e. s/bt/backtrace/
> +++ b/common/trace_backtrace.hpp
That's a lot of code in the header file. The library resolution in
particular should go into a cpp file.
> +static int SOopened = 0;
> +static void* (*threadself)(void);
> +static FILE* log;
> +static char* buf;
> +static size_t size;
> +static void (*cTarget)(DebugOutputTarget*, FILE*);
> +static void (*dumpSelf)(const DebugOutputTarget*, void*);
> +static FILE* (*open_memstream_exp)(char**, size_t*);
> +static DebugOutputTarget t;
That's a /lot/ of global static's, it really should be reduced a bit.
> args(_sig->num_args),
> ret(0),
> - flags(_flags) {
> + flags(_flags),
> + bt() {
> }
That's unnecessary. std::string will be default constructed anyway.
> + QString bt() const;
> + void setbt(QString bt);
QString backtrace() const;
void setBacktrace(const QString &backtrace);
> +struct pstring {
> + const char* s;
> + int n;
> + pstring(const char* s, int n)
> + {
> + this->s = s;
> + this->n = n;
> + }
> + bool operator<(const pstring q2) const
> + {
> + return memcmp(s, q2.s, n < q2.n? n : q2.n) < 0; /* return true, if strlen(s); */
> + }
> +};
Is that supposed to represent offsets in the backtrace store?
> +
> +#define PREFIX_BUF_SIZE (PREFIX_MAX_FUNC_NAME * MAX_BT_FUNC)
> +
> +class Prefixes {
Is that a backtrace store? They both should be named better than that.
> +static Prefixes prefixes(APITRACE_FNAMES_SOURCE);
Name, plus lets not make it a static global.
> +
> +bool egl_func_backtrace_is_needed(const char* fname)
> +{
> + return prefixes.contain(fname);
> + /*
> + static bool nofirst = false;
Why is that function mostly commented out?
> + # 0-4 are reserved to memcpy, malloc, free, realloc, eglFakeBacktrace
You seemed to have named it glFakeBacktrace...
z
More information about the apitrace
mailing list