Re: [Minios-devel] [UNIKRAFT PATCH v3 5/7] include/essentials: Provide __constructor macro

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

>> I am fine with 2 macros, but I failed to figure out what 'p' in the end
>> stands for :). Maybe name it __constructor_lvl()?
>
> ;-) Okay, I'll go for __constructor_prio()
> You want a resubmission for this or shall I go ahead?
I am ok with current version of the patch, which does not have second
macro at all. It was Sharan's complain :).

If you want to reissue this patch, then I would prefer
__constructor_prio() over __constructorp(). That's all.

Simon Kuenzer <simon.kuenzer@xxxxxxxxx> writes:

> On 01.06.2018 13:30, Yuri Volchkov wrote:
>> Simon Kuenzer <simon.kuenzer@xxxxxxxxx> writes:
>> 
>>> On 01.06.2018 11:31, Sharan Santhanam wrote:
>>>>
>>>>
>>>> On 05/30/2018 04:18 PM, Simon Kuenzer wrote:
>>>>>
>>>>>
>>>>> On 23.05.2018 15:00, Sharan Santhanam wrote:
>>>>>> Hello Simon,
>>>>>>
>>>>>>
>>>>>> Please find my comments in line.
>>>>>>
>>>>>>
>>>>>> On 05/22/2018 02:20 PM, Simon Kuenzer wrote:
>>>>>>> Provide a constructor attribure macro for marking a
>>>>>>> function symbol as constructor. The linker/compiler
>>>>>>> is going to populate a function pointer of it to
>>>>>>> the init_array section of the binary.
>>>>>>>
>>>>>>> Signed-off-by: Simon Kuenzer<simon.kuenzer@xxxxxxxxx>
>>>>>>> ---
>>>>>>>    include/uk/essentials.h | 13 +++++++++++++
>>>>>>>    1 file changed, 13 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/uk/essentials.h b/include/uk/essentials.h
>>>>>>> index f6cc6ea..3d1b705 100644
>>>>>>> --- a/include/uk/essentials.h
>>>>>>> +++ b/include/uk/essentials.h
>>>>>>> @@ -73,6 +73,19 @@ extern "C" {
>>>>>>>    #ifndef __align
>>>>>>>    #define __align(bytes)         __attribute__((aligned(bytes)))
>>>>>>>    #endif
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * Mark a function as constructor
>>>>>>> + * The compiler/linker will populate a function pointer
>>>>>>> + * (sorted by priority) to the init_array section
>>>>>>> + *
>>>>>>> + * @param lvl
>>>>>>> + *   Priority level (101 (earliest)...onwards (latest))
>>>>>>> + */
>>>>>>> +#ifndef __constructor
>>>>>>> +#define __constructor(lvl) __attribute__ ((constructor (lvl)))
>>>>>>> +#endif
>>>>>> The lvl parameter is an optional parameter. We are forcing the user
>>>>>> to use constructor with priority as the default case. Is there any
>>>>>> reason behind it?
>>>>>
>>>>> I wanted to expose the richer functionality with the macro and making
>>>>> the programmer aware that there is a order in which constructors are
>>>>> called. The user can still use `__attribute__((constructor))` but this
>>>>> one would have lower priority than the ones with a level parameter.
>>>>>
>>>>> I am fine if you say a macro for both should be there. Is this the case?
>>>>>
>>>>
>>>> I agree in exposing the lvl parameter, but if the user of the library is
>>>> going to directly "__attribute__((constructor))" we may have
>>>> difficulties in enforcing the order of the constructor, if that is
>>>> necessary. So I do not like that part as a part of the application
>>>> porting to Unikraft might require the user to know the order of the
>>>> constructor across the different libraries.
>>>>
>> 
>> Actually, if an application needs __constructor, it is already far
>> beyond normal api. This means one already have to think about lower
>> level things. In my opinion thinking about levels is just another one
>> tiny step down, which does not make that big difference.
>> 
>>>
>>> If the user is using __attribute__((constructor)) directly (without
>>> lvl), such functions will be called after the prioritized ones.
>>> Constructors with a priority level always called before the non-leveld ones.
>>>
>>> Would this avoid difficulties?
>>>
>>> /**
>>>    * Mark a function as constructor
>>>    * The compiler/linker will populate a function pointer
>>>    * to the init_array section
>>>    */
>>> #ifndef __constructor
>>> #define __constructor __attribute__ ((constructor))
>>> #endif
>>>
>>> /**
>>>    * Mark a function as constructor with priority
>>>    * The compiler/linker will populate a function pointer
>>>    * (sorted by priority) to the init_array section
>>>    * Prioritized constructors are called before
>>>    * non-prioritized ones
>>>    *
>>>    * @param lvl
>>>    *   Priority level (101 (earliest)...onwards (latest))
>>>    */
>>> #ifndef __constructorp
>>> #define __constructorp(lvl) __attribute__ ((constructor (lvl)))
>>> #endif
>> I am fine with 2 macros, but I failed to figure out what 'p' in the end
>> stands for :). Maybe name it __constructor_lvl()?
>
> ;-) Okay, I'll go for __constructor_prio()
> You want a resubmission for this or shall I go ahead?
>
>>>
>>>>>>> +
>>>>>>>    #else
>>>>>>>    /* TO BE DEFINED */
>>>>>>>    #endif /* __GNUC__ */
>>>>>>
>>>>>> Thanks & Regards
>>>>>> Sharan Santhanam
>>>>>
>>>>
>>>> Thanks & Regards
>>>> Sharan
>>>
>>> _______________________________________________
>>> Minios-devel mailing list
>>> Minios-devel@xxxxxxxxxxxxxxxxxxxx
>>> https://lists.xenproject.org/mailman/listinfo/minios-devel
>> 

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