Re: [Minios-devel] [UNIKRAFT PATCHv4 07/43] build: Add a makefile function to check and set flags for valid gcc

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Simon,

> -----Original Message-----
> From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Sent: 2018年7月10日 23:35
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 07/43] build: Add a makefile
> function to check and set flags for valid gcc
> 
> On 06.07.2018 11:03, Wei Chen wrote:
> > Some times, we will add special flags to GCC to do optimization. For
> > instance, we can use this function to check valid GCC and set flags
> > to do processors optimization. In order to avoid any definition
> > clashes, we define this function in support/build/Makefile.rules.
> >
> > Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> > ---
> >   support/build/Makefile.rules | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/support/build/Makefile.rules b/support/build/Makefile.rules
> > index f71fd6e..261b8f5 100644
> > --- a/support/build/Makefile.rules
> > +++ b/support/build/Makefile.rules
> > @@ -42,6 +42,14 @@ $(if $(call test_gcc_version,$(1),$(2)),,\
> >        $(error Require GCC version >= $(1).$(2) found
> $(CC_VER_MAJOR).$(CC_VER_MINOR)))
> >   endef
> >
> > +# set_flags_if_gcc_version_ge $gcc_major,$gcc_minor,$flags
> > +define set_flags_if_gcc_version_ge =
> > +$(call error_if_gcc_version_lt,$(1),$(2))
> 
> Oh, I would not expect from the set_flags_if_gcc_version_ge function
> name that it is going to let Make stop execution and returning an error.
> I would expect that it sets only the flags if the condition is met and
> ignore it otherwise. Hum...
> 

Oh, yes. We should not stop the make execution. It's my mistake, I will
fix it.

> > +ASFLAGS-$(1)  += $(3)
> > +CFLAGS-$(1)   += $(3)
> > +CXXFLAGS-$(1) += $(3)
> 
> I had a chat with Sharan and we actually prefer forgetting about a
> function that sets flags to all these three variables. We get the
> feeling that the function is too powerful with the cost not to
> understand anymore what it does.
> 
> Instead of doing this:
> 
> $(call set_flags_if_gcc_version_ge,4,5,-my_flag)
> 
> ...we think about a usage flow like this (as equal expression):
> 
> $(call error_if_gcc_version_lt,4,5)
> 
> ASFLAGS-$(call gcc_version_ge,4,5)  += -my_flag
> CFLAGS-$(call gcc_version_ge,4,5)   += -my_flag
> CXXFLAGS-$(call gcc_version_ge,4,5) += -my_flag
> 
> I know you have to write more for this but we think this is easier to
> get without needing to lookup what the Make functions actually do.
> gcc_version_ge is the function you have called test_gcc_version in your
> previous patch and which result in a `y` if the GCC version is greater
> or equal to the parameters.
> 
> I am sorry to suggest you again another change of this. I know that we
> agreed on the system you implemented right now. What do you think?
> 

It doesn't matter. This new way seems more sensible. I will implement
it in next version.

> 
> > +endef
> > +
> >
> ##############################################################################
> ##
> >   #
> >   # Paths and Filenames
> >
> 
> Thanks,
> 
> Simon
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel