Re: [Minios-devel] [UNIKRAFT PATCH 2/8] lib/ukdebug: Register a tracepoint

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Hi,

Costin Lupu <costin.lup@xxxxxxxxx> writes:

> Hi Yuri,
>
> Please see my comments inline.
>
> On 5/10/19 9:29 PM, Yuri Volchkov wrote:
>>        Build code with assertions.
>>  
>> +menuconfig LIBUKDEBUG_TRACEPOINTS
>> +    bool "enable tracepoints"
>
> If there will be a v2, please properly edit this string, inline with the
> other config options of Unikraft.
Could you be more specific about this please?

>> +#define __UK_TRACE_GET_TYPE(arg) (                                  \
>> +    __builtin_types_compatible_p(typeof(arg), const char *) *       \
>> +            __UK_TRACE_ARG_STRING +                                 \
>> +    __builtin_types_compatible_p(typeof(arg), char *) *             \
>> +            __UK_TRACE_ARG_STRING +                                 \
>> +    0)
>
> Do we really need this +0 ?
That's just to make it look more beautiful. That all lines of
__builtin_types look the same. And also for better patches extending
this functionality. Those patches will have just +lines for added
types.

That is just a matter of a taste. I would keep it this way if you do not
mind too much.

>> +#define __UK_TRACE_REG(NR, regname, trace_name, fmt, ...)   \
>> +    UK_CTASSERT(sizeof(#trace_name) < 255);                 \
>> +    UK_CTASSERT(sizeof(fmt) < 255);                         \
>> +    __attribute((__section__(".uk_tracepoints_list")))      \
>> +    static struct {                                         \
>> +            uint32_t magic;                                 \
>> +            uint32_t size;                                  \
>> +            uint64_t cookie;                                \
>> +            uint8_t args_nr;                                \
>> +            uint8_t name_len;                               \
>> +            uint8_t format_len;                             \
>> +            uint8_t sizes[NR];                              \
>> +            uint8_t types[NR];                              \
>
> If we get rid of the n parameter in FOREACH,  then we can also get rid
> of the NR parameter. I don't feel strong about this one, it's your call.
As I said already, this NR is for simpler macro. NR_ARGS everywhere is
really does look bad. I had it originally and later I introduced the
NR. I think you have an access to the earlier version, so you probably
could compare.


-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel