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

[Costin Lupu <costin.lup@xxxxxxxxx>]

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.
> 
> 

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