Re: [PATCH v2] x86: Remove libopcodes dependency

[Jan Beulich <jbeulich@suse.com>]

On 30.11.2022 23:22, H.J. Lu wrote:
> On Tue, Nov 29, 2022 at 10:58 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 30.11.2022 01:06, H.J. Lu wrote:
>>> On Tue, Nov 29, 2022 at 11:38 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Tue, Nov 29, 2022 at 1:22 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 29.11.2022 00:49, H.J. Lu wrote:
>>>>>> On Thu, Nov 24, 2022 at 2:19 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>> On 22.11.2022 19:19, H.J. Lu wrote:
>>>>>>>> --- a/gas/Makefile.am
>>>>>>>> +++ b/gas/Makefile.am
>>>>>>>> @@ -446,6 +446,12 @@ development.exp: $(BFDDIR)/development.sh
>>>>>>>>       $(EGREP) "(development|experimental)=" $(BFDDIR)/development.sh  \
>>>>>>>>         | $(AWK) -F= '{ print "set " $$1 " " $$2 }' > $@
>>>>>>>>
>>>>>>>> +$(srcdir)/../opcodes/i386-init.h $(srcdir)/../opcodes/i386-tbl.h: \
>>>>>>>> +     @MAINT@ $(srcdir)/../opcodes/i386-opc.tbl \
>>>>>>>> +     $(srcdir)/../opcodes/i386-reg.tbl \
>>>>>>>> +     $(srcdir)/../opcodes/i386-opc.h
>>>>>>>> +     cd ../opcodes; make gen-i386-tbl
>>>>>>>
>>>>>>> I've made a patch to gas/Makefile.am as you have requested in reply to
>>>>>>> my series. I will want to put that through some more testing, so I will
>>>>>>> submit a v3 of that only a little later (and of course only unless you
>>>>>>> submit a v2 of your patch earlier that I would also end up being okay
>>>>>>> with). In the course of doing so I noticed a few more issues with your
>>>>>>> change:
>>>>>>>
>>>>>>> For one I don't think you can put @MAINT@ on a continued line, as the
>>>>>>> line continuation might then be hidden when @MAINT@ expands to #. The
>>>>>>> list of dependencies wants expressing via a variable, which would then
>>>>>>> be used immediately after @MAINT@ without any line continuation
>>>>>>> following.
>>>>>>
>>>>>> Fixed.
>>>>>
>>>>> No, the same problem is still there. You either need to use a very long
>>>>> line, or you need to introduce a variable holding the list of prereqs,
>>>>> like I've done in my series.
>>>>
>>>> I got
>>>>
>>>> $(srcdir)/../opcodes/i386-init.h $(srcdir)/../opcodes/i386-tbl.h:
>>>> $(srcdir)/../opcodes/i386-opc.tbl \
>>>>         $(srcdir)/../opcodes/i386-reg.tbl \
>>>>         $(srcdir)/../opcodes/i386-opc.h \
>>>>         $(srcdir)/../opcodes/i386-gen.c
>>>>         $(MAKE) -C ../opcodes gen-i386-tbl
>>>>
>>>> and
>>>>
>>>> $(srcdir)/../opcodes/i386-init.h $(srcdir)/../opcodes/i386-tbl.h: #
>>>> $(srcdir)/../opcodes/i386-opc.tbl \
>>>>         $(srcdir)/../opcodes/i386-reg.tbl \
>>>>         $(srcdir)/../opcodes/i386-opc.h \
>>>>         $(srcdir)/../opcodes/i386-gen.c
>>>>         $(MAKE) -C ../opcodes gen-i386-tbl
>>>>
>>>> I didn't run into any problems.
>>>>
>>>>>>> And then your rule / dependency won't be enough on a "maintainer-clean"
>>>>>>> tree, i.e. when the generated headers aren't there at all, and when
>>>>>>> config/.deps/tc-i386.Po is still empty. In that case nothing would
>>>>>>> trigger their generation; an explicit dependency of config/tc-i386.o on
>>>>>>> these headers needs adding here.
>>>>>>
>>>>>> Fixed.
>>>>>>
>>>>>>> Finally you're missing a dependency of the generated headers on
>>>>>>> i386-gen.c.
>>>>>>
>>>>>> They have a dependency on i386-gen which depends on i386-gen.c.
>>>>>
>>>>> In opcodes/, yes, but talk was about the rule in gas/. Yet despite
>>>>> your comment above I see that you've added the missing dependency.
>>>>>
>>>>>> Here is the v2 patch.
>>>>>
>>>>> As said in reply to v1 - my objection to this particular use of make
>>>>> recursion remains.
>>>>
>>>> The gen-i386-tbl rule doesn't touch any files used by libopcodes.
>>>
>>> There are
>>>
>>> bfd/Makefile.in:   ($(am__cd) $$subdir && $(MAKE) $(AM_MAKEFLAGS)
>>> $$local_target) \
>>> binutils/Makefile.in:   ($(am__cd) $$subdir && $(MAKE) $(AM_MAKEFLAGS)
>>> $$local_target) \
>>> gas/Makefile.in:   ($(am__cd) $$subdir && $(MAKE) $(AM_MAKEFLAGS)
>>> $$local_target) \
>>
>> I'm glad you spotted this yourself - I was just about to point out
>> that my concern isn't only a theoretical one, because of the
>> generated makefiles we use. You don't draw any conclusion though:
>> Are you willing to accept my approach, specifically reworked to
>> account for an extra request of yours? Or are you meaning to make
>> another attempt yourself, avoiding the undue make recursion?
>>
> 
> Calling $(MAKE) in Makefile is very normal in binutils-gdb.

Any further examples where this isn't done strictly top-down, or in
a properly controlled way within a single directory? I can indeed
spot many instances of $(MAKE), but none matching the pattern you
intend to introduce.

> I don't see anything wrong with my approach.  I prefer my
> approach.

I understand you prefer it, but given you've found what's wrong
yourself, I'm puzzled by "I don't see anything wrong with my
approach". Or wait - looks like I misread what you pointed out
above: That looks to be something from the top level Makefile.
Yet then again I can't spot any such in my build trees. Where is
that excerpt from? (I can spot somewhat similar patterns, but
they're used strictly to recurse _down_, just like looks to be
the case with what you've quoted.)

In any event, a practical manifestation of the issue I've said
I'm concerned of is this: If a 2nd party anywhere in the tree did
the same you do, and if the two $(MAKE) invocations then raced
with one another, then it's plain undefined what would happen to
the recursed-into Makefile when that needs re-generating for
whatever reason. Hence $(MAKE) recursion should only occur
strictly top-down, with (suitably controlled) recursion _within_
a directory possibly being okay (would need proving for every
individual instance).

Jan