[PATCH] Backtrace for android and linux

[Zack Rusin]

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