Re: [PATCH v8 1/2] mk:Simplify the ifdefs in rte.app.mk

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Keith,

On 05/13/2015 04:43 PM, Wiles, Keith wrote:
> 
> 
> On 5/13/15, 9:28 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
> 
>>
>> On 05/13/2015 04:04 PM, Wiles, Keith wrote:
>>>
>>>
>>> On 5/13/15, 8:56 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
>>>
>>>> Hi Keith,
>>>>
>>>> On 05/13/2015 03:17 PM, Wiles, Keith wrote:
>>>>>>>
>>>>>>>     endif # ifeq ($(NO_AUTOLIBS),)
>>>>>>>
>>>>>>> -LDLIBS += $(CPU_LDLIBS)
>>>>>>> +LDLIBS += $(_LDLIBS-y) $(EXTRA_LDLIBS)
>>>>>>>
>>>>>>
>>>>>> As discussed in the previous mail, all things that are about
>>>>>> EXTRA_LDLIBS should be moved in the second patch. Therefore,
>>>>>> the title of the second patch should not be "update doc...", but
>>>>>> something like "mk: introduce EXTRA_LDLIBS...".
>>>>>>
>>>>>> By the way, I missed that before, but it seems that your
>>>>>> patch removes CPU_LDLIBS, I don't think it's correct.
>>>>>
>>>>> I found no reference to CPU_LDLIBS in the docs or code other then then
>>>>> one
>>>>> line. We now have EXTRA_LDLIBS for the command line, right?
>>>>
>>>> Yes, but your patch says "simplify the ifdef". Removing
>>>> a variable (even if it is not used) in this patch is not
>>>> a good idea.
>>>>
>>>> Now, the CPU_CFLAGS, CPU_LDFLAGS, CPU_LDLIBS can be defined internally
>>>> by the rte.vars.mk in mk/arch/ or mk/machine/ directories.
>>>
>>> No docs for CPU_LDLIBS or reference to that variable, which means it
>>> does
>>> not exist, right?
>>> If it was used or documented then I would agree. Having magic variables
>>> is
>>> not a good idea. I will add the CPU_LDLIBS in to the line, but someone
>>> will have to document that variable.
>>
>> First, this variable is internal to DPDK framework. It is not
> 
> Not referenced in the code any place except the one line and not
> referenced in the docs, which means no one used that variable. By
> definition this is some magic variable that someone could use only because
> he knew about that variable.
> 
> As I stated before, I will add that variable back, but it must be
> documented, correct?
> 
> Look at the code a number of CPU_XXXX flags are commented about in the
> code.

It could be added at the same place than other CPU_* variables,
in another patch.


Olivier



> 
> machine/power8/rte.vars.mk:39:#   - can define CPU_CFLAGS variable
> (overridden by cmdline value) that
> machine/power8/rte.vars.mk:41:#   - can define CPU_LDFLAGS variable
> (overridden by cmdline value) that
> machine/power8/rte.vars.mk:43:#   - can define CPU_ASFLAGS variable
> (overridden by cmdline value) that
> machine/power8/rte.vars.mk:53:# CPU_CFLAGS =
> machine/power8/rte.vars.mk:54:# CPU_LDFLAGS =
> machine/power8/rte.vars.mk:55:# CPU_ASFLAGS =
> machine/wsm/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/wsm/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/wsm/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/wsm/rte.vars.mk:54:# CPU_CFLAGS =
> machine/wsm/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/wsm/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/native/rte.vars.mk:40:#   - can define CPU_CFLAGS variable
> (overriden by cmdline value) that
> machine/native/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/native/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/native/rte.vars.mk:54:# CPU_CFLAGS =
> machine/native/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/native/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/native/rte.vars.mk:66:  CPU_SSE42_SUPPORT = $(shell grep SSE4\.2
> /var/run/dmesg.boot 2>/dev/null)
> machine/native/rte.vars.mk:67:  ifneq ($(CPU_SSE42_SUPPORT),)
> machine/atm/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/atm/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/atm/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/atm/rte.vars.mk:54:# CPU_CFLAGS =
> machine/atm/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/atm/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/hsw/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/hsw/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/hsw/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/hsw/rte.vars.mk:54:# CPU_CFLAGS =
> machine/hsw/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/hsw/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/ivb/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/ivb/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/ivb/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/ivb/rte.vars.mk:54:# CPU_CFLAGS =
> machine/ivb/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/ivb/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/snb/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/snb/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/snb/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/snb/rte.vars.mk:54:# CPU_CFLAGS =
> machine/snb/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/snb/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/nhm/rte.vars.mk:40:#   - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/nhm/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/nhm/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/nhm/rte.vars.mk:54:# CPU_CFLAGS =
> machine/nhm/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/nhm/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/default/rte.vars.mk:40:#   - can define CPU_CFLAGS variable
> (overriden by cmdline value) that
> machine/default/rte.vars.mk:42:#   - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/default/rte.vars.mk:44:#   - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/default/rte.vars.mk:54:# CPU_CFLAGS =
> machine/default/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/default/rte.vars.mk:56:# CPU_ASFLAGS =
> rte.lib.mk:43:CPU_LDFLAGS += --version-script=$(SRCDIR)/$(EXPORT_MAP)
> rte.lib.mk:67:LD := $(CC) $(CPU_CFLAGS)
> rte.lib.mk:68:_CPU_LDFLAGS := $(call linkerprefix,$(CPU_LDFLAGS))
> rte.lib.mk:70:_CPU_LDFLAGS := $(CPU_LDFLAGS)
> rte.lib.mk:82:O_TO_S = $(LD) $(_CPU_LDFLAGS) -shared $(OBJS-y)
> -Wl,-soname,$(LIB) -o $(LIB)
> rte.app.mk:144:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
> arch/x86_64/rte.vars.mk:39:#   - define CPU_CFLAGS variable (overriden by
> cmdline or previous
> arch/x86_64/rte.vars.mk:41:#   - define CPU_LDFLAGS variable (overriden by
> cmdline or previous
> arch/x86_64/rte.vars.mk:43:#   - define CPU_ASFLAGS variable (overriden by
> cmdline or previous
> arch/x86_64/rte.vars.mk:55:CPU_CFLAGS  ?= -m64
> arch/x86_64/rte.vars.mk:56:CPU_LDFLAGS ?=
> arch/x86_64/rte.vars.mk:57:CPU_ASFLAGS ?= -felf64
> arch/x86_64/rte.vars.mk:59:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
> CPU_ASFLAGS
> arch/i686/rte.vars.mk:39:#   - define CPU_CFLAGS variable (overriden by
> cmdline or previous
> arch/i686/rte.vars.mk:41:#   - define CPU_LDFLAGS variable (overriden by
> cmdline or previous
> arch/i686/rte.vars.mk:43:#   - define CPU_ASFLAGS variable (overriden by
> cmdline or previous
> arch/i686/rte.vars.mk:55:CPU_CFLAGS  ?= -m32
> arch/i686/rte.vars.mk:56:CPU_LDFLAGS ?= -melf_i386
> arch/i686/rte.vars.mk:57:CPU_ASFLAGS ?= -felf
> arch/i686/rte.vars.mk:59:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
> CPU_ASFLAGS
> arch/x86_x32/rte.vars.mk:39:#   - define CPU_CFLAGS variable (overridden
> by cmdline or previous
> arch/x86_x32/rte.vars.mk:41:#   - define CPU_LDFLAGS variable (overridden
> by cmdline or previous
> arch/x86_x32/rte.vars.mk:43:#   - define CPU_ASFLAGS variable (overridden
> by cmdline or previous
> arch/x86_x32/rte.vars.mk:54:CPU_CFLAGS  ?= -mx32
> arch/x86_x32/rte.vars.mk:55:CPU_LDFLAGS ?= -melf32_x86_64
> arch/x86_x32/rte.vars.mk:56:#CPU_ASFLAGS ?= -felf64
> arch/x86_x32/rte.vars.mk:59:ifneq ($(shell echo | $(CC) $(CPU_CFLAGS) -E -
> 2>/dev/null 1>/dev/null && echo 0), 0)
> arch/x86_x32/rte.vars.mk:63:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
> CPU_ASFLAGS
> arch/ppc_64/rte.vars.mk:35:CPU_CFLAGS  ?= -m64 -DRTE_CACHE_LINE_SIZE=128
> arch/ppc_64/rte.vars.mk:36:CPU_LDFLAGS ?=
> arch/ppc_64/rte.vars.mk:37:CPU_ASFLAGS ?= -felf64
> arch/ppc_64/rte.vars.mk:39:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
> CPU_ASFLAGS
> target/generic/rte.vars.mk:46:#   - can define CPU_CFLAGS variable
> (overriden by cmdline value) that
> target/generic/rte.vars.mk:48:#   - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> target/generic/rte.vars.mk:50:#   - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> target/generic/rte.vars.mk:64:#   - define CPU_CFLAGS variable (overriden
> by cmdline or previous
> target/generic/rte.vars.mk:66:#   - define CPU_LDFLAGS variable (overriden
> by cmdline or previous
> target/generic/rte.vars.mk:68:#   - define CPU_ASFLAGS variable (overriden
> by cmdline or previous
> target/generic/rte.vars.mk:110:CFLAGS := $(CPU_CFLAGS) $(EXECENV_CFLAGS)
> $(TOOLCHAIN_CFLAGS) $(MACHINE_CFLAGS)
> target/generic/rte.vars.mk:114:LDFLAGS := $(CPU_LDFLAGS)
> $(EXECENV_LDFLAGS) $(TOOLCHAIN_LDFLAGS) $(MACHINE_LDFLAGS)
> target/generic/rte.vars.mk:118:ASFLAGS := $(CPU_ASFLAGS)
> $(EXECENV_ASFLAGS) $(TOOLCHAIN_ASFLAGS) $(MACHINE_ASFLAGS)
> rte.sharelib.mk:52:LD := $(CC) $(CPU_CFLAGS)
> rte.sharelib.mk:53:O_TO_S = $(LD) $(call linkerprefix,$(CPU_LDFLAGS)) \
> rte.sharelib.mk:56:O_TO_S = $(LD) $(CPU_LDFLAGS) \
> 
> 
> 
>> documented, because the goal of the documentation is not to
>> document all internal variables. In one word, it's not a magic
>> variable at all, it is simply a variable.
>>
>> Now, the heart of the matter is that your patch silently remove
>> this line. This is not acceptable and that's why I'm commenting
>> on it. The goal of patch splitting and proper title is to separate
>> features, and win time when searching in the git log for the cause
>> of a problem.
>>
>> This is the same problem with EXTRA_LDLIBS. How can you justify
>> having 2 patches:
>> - "simplify the ifdef" that also adds the EXTRA_LDLIBS
>>  then
>> - "update the documentation for EXTRA_LDLIBS"
>>
>> Regards,
>> Olivier
>>
>>
>>
>>>>
>>>> Regards,
>>>> Olivier
>>>>
>>>
>>
> 
>