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

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Ah, ok. Will fix that.

Costin Lupu <costin.lup@xxxxxxxxx> writes:

> On 5/29/19 2:14 PM, Yuri Volchkov wrote:
>> 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?
>> 
>
> I mean all the other titles in menuconfig for ukdebug start with upper
> case, this is the only one that stands out.
>
>>>> +#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