Diego Biurrun <[email protected]> writes:
> On Sun, Jul 22, 2012 at 01:50:03AM +0100, Mans Rullgard wrote:
>> This adds a full identification probe of CC, AS, LD and HOSTCC,
>> and sets up correct flags and dependency tracking for each.
>
> This is shaping up to be a big step forward. But it's very tricky
> code with a lot of subtle name mangling, not easy to review at all...
>
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -25,14 +25,15 @@ ALLFFLIBS = avcodec avdevice avfilter avformat
>> avresample avutil swscale
>> IFLAGS := -I. -I$(SRC_PATH)
>> CPPFLAGS := $(IFLAGS) $(CPPFLAGS)
>> CFLAGS += $(ECFLAGS)
>> -CCFLAGS = $(CFLAGS)
>> +CCFLAGS = $(CPPFLAGS) $(CFLAGS)
>> +ASFLAGS := $(CPPFLAGS) $(ASFLAGS)
>> YASMFLAGS += $(IFLAGS) -I$(SRC_PATH)/libavutil/x86/ -Pconfig.asm
>> HOSTCFLAGS += $(IFLAGS)
>> LDFLAGS := $(ALLFFLIBS:%=-Llib%) $(LDFLAGS)
>
> The header compilation rule still lists both CPPFLAGS and CFLAGS
> explicitly and should likely be switched to just CCFLAGS.
We can do that later. I didn't want to include anything not strictly
necessary in this patch.
>> define COMPILE
>> - $($(1)DEP)
>> - $($(1)) $(CPPFLAGS) $($(1)FLAGS) $($(1)_DEPFLAGS) -c $($(1)_O) $<
>> + $(call $(1)DEP,$(1))
>> + $($(1)) $($(1)FLAGS) $($(1)_DEPFLAGS) -c $($(1)_O) $<
>> endef
>
> FOOFLAGS vs. FOO_DEPFLAGS is an inconsistency.
Inconsistent with what?
> Given how involved all of this is getting, such inconsistencies start
> to become a problem IMO.
>
> What about the rules for HOSTOBJS and HOSTPROGS in common.mak? They
> still have hardcoded "-o $@" syntax in them.
That's changed in patch 10/10 from the original series. Again, it
wasn't necessary to do it in this patch, so I separated it for ease of
review.
>> --- a/configure
>> +++ b/configure
>> @@ -1725,7 +1725,9 @@ ldflags_filter=echo
>>
>> AS_O='-o $@'
>> CC_O='-o $@'
>> +LD_O='-o $@'
>>
>> +HOSTCC_O='-o $@'
>> host_cflags='-D_ISOC99_SOURCE -D_XOPEN_SOURCE=600 -O3 -g'
>> host_libs='-lm'
>> host_cflags_filter=echo
>
> IMO it makes more sense to keep the _O variables together, whatever ...
I wasn't sure which grouping you'd consider stronger. I supposed I
guessed incorrectly.
>> @@ -1735,8 +1737,8 @@ target_path='$(CURDIR)'
>>
>> # since the object filename is not given with the -MM flag, the compiler
>> # is only able to print the basename, and we must add the path ourselves
>> -DEPEND_CMD='$(DEPCC) $(DEPFLAGS) $< | sed -e "/^\#.*/d" -e
>> "s,^[[:space:]]*$(*F)\\.o,$(@D)/$(*F).o," > $(@:.o=.d)'
>> -DEPFLAGS='$(CPPFLAGS) $(CFLAGS) -MM'
>> +DEPEND_CMD='$(DEP$(1)) $(DEP$(1)FLAGS) $($(1)DEP_FLAGS) $< | sed -e
>> "/^\#.*/d" -e "s,^[[:space:]]*$(*F)\\.o,$(@D)/$(*F).o," > $(@:.o=.d)'
>> +DEPFLAGS='-MM'
>
> Please rename DEPEND_CMD to DEPCMD for consistency.
Will do.
>> # find source path
>> if test -f configure; then
>> @@ -2036,120 +2038,150 @@ tms470_flags(){
>> done
>> }
>>
>> -if $cc -v 2>&1 | grep -q '^gcc.*LLVM'; then
>> + if $_cc -v 2>&1 | grep -q '^gcc.*LLVM'; then
>
> nit: The extra spaces here are slightly silly. Maybe you can drop them
> while you're at it.
Fine.
>> +set_ccvars(){
>> + eval ${1}_O=\${_cc_o-\${${1}_O}}
>> +
>> + if [ -n "$_depflags" ]; then
>> + eval ${1}_DEPFLAGS=\$_depflags
>> + else
>> + eval ${1}DEP=\${_DEPCMD:-\$DEPEND_CMD}
>> + eval ${1}DEP_FLAGS=\${_DEPFLAGS:-\$DEPFLAGS}
>> + eval DEP${1}FLAGS=\$_flags
>> + fi
>> +}
>
> Here's where the underscore inconsistency becomes visible.
> It does get confusing.
>
> Through uppercasing a variable you convey the information whether or
> not a compiler can generate dependency information as a side-effect
> of compilation. Just setting a flag and testing that would be a
> more obvious way of achieving the same thing.
No, that is not how it works.
If _depflags is set, it means the compiler can generate dependencies
while compiling and will do so if $_depflags is added to the command
line.
If _depflags is not set, a separate pass is needed for dependency
generation. In this case, _DEPCMD, if set, provides a template for a
full command to generate dependencies. If _DEPCMD is not set, the
global DEPCMD (once renamed per your request) is correct for this
compiler (most of them work with the same command).
In all cases, _DEPFLAGS contains flags triggering a standalone
dependency generation if different from the default DEPFLAGS.
Using a separate indicator variable would only make things more complex
and fragile. It would be akin to using a separate C variable to
indicate whether a pointer is null or not.
> I also wonder if FOO_DEPFLAGS should be kept separate of the normal
> FOOFLAGS at all when generating dependency information as a side-effect.
They need to be separate since the FOO_DEPFLAGS typically include make
variables that wouldn't work when running the compiler from configure
(for testing various things). They could of course be combined in the
makefile, but I think it is clearer to keep them separate.
>> @@ -3370,9 +3419,10 @@ SLIBPREF=$SLIBPREF
>> SLIBSUF=$SLIBSUF
>> EXESUF=$EXESUF
>> EXTRA_VERSION=$extra_version
>> -DEPFLAGS=$DEPFLAGS
>> CCDEP=$CCDEP
>> +CCDEP_FLAGS=$CCDEP_FLAGS
>> ASDEP=$ASDEP
>> +ASDEP_FLAGS=$ASDEP_FLAGS
>> CC_DEPFLAGS=$CC_DEPFLAGS
>> AS_DEPFLAGS=$AS_DEPFLAGS
>> HOSTCC=$host_cc
>
> CCDEP_FLAGS vs. CC_DEPFLAGS ...
>
> Heads are starting to spin everywhere ...
OK, here's a rundown:
CCDEP command template for standalone dependency generation
DEPCC compiler to use for standalone dependency generation
DEPCCFLAGS random compiler-specific flags + preprocessor flags
CCDEP_FLAGS flags requesting standalone dependency generation
CC_DEPFLAGS flags requesting dependency generation while compiling
In the above names CC can be replaced with AS or HOSTCC.
> I think CCDEP_FLAGS should be CCDEPFLAGS. In all other cases the flags
> required when invoking FOO do not have an underscore in their name, cf
> CCFLAGS, ASFLAGS and similar.
Those variables are not equivalent. The best equivalent is DEPCCFLAGS
which already has no underscore.
>> @@ -3380,6 +3430,12 @@ HOSTCFLAGS=$host_cflags
>> HOSTEXESUF=$HOSTEXESUF
>> HOSTLDFLAGS=$host_ldflags
>> HOSTLIBS=$host_libs
>> +DEPHOSTCC=$host_cc
>
> What about the case when deps are not generated with the host compiler?
There is no such case. I have never come across a host compiler unable
to generate dependencies.
> For the target compiler we set this separately:
>
> DEPCC=$dep_cc
>
>> +DEPHOSTCCFLAGS=$DEPHOSTCCFLAGS \$(CPPFLAGS)
>
> As you mentioned on IRC, these are the target CPPFLAGS.
Yes, I'm replacing it with HOSTCCFLAGS instead. There is (currently) no
separate variable for host preprocessor flags, and including all the
flags does no harm.
I also noticed yet another problem. I'll send an updated patch soon.
--
Måns Rullgård
[email protected]
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
