Re: [PATCH] build: add crypto/ to SUBDIRS

[Michal Orzel <michal.orzel@xxxxxxx>]

On 27/02/2023 16:00, Jan Beulich wrote:
> 
> 
> On 27.02.2023 15:46, Michal Orzel wrote:
>> On 27/02/2023 14:54, Jan Beulich wrote:
>>> On 27.02.2023 14:41, Michal Orzel wrote:
>>>> On 27/02/2023 11:10, Jan Beulich wrote:
>>>>> On 27.02.2023 10:53, Michal Orzel wrote:
>>>>>> --- a/xen/Makefile
>>>>>> +++ b/xen/Makefile
>>>>>> @@ -589,7 +589,7 @@ $(TARGET): outputmakefile FORCE
>>>>>>       $(Q)$(MAKE) $(build)=. 
>>>>>> arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>>>>>>       $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
>>>>>> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>>>>>>
>>>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>>>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common crypto drivers lib test
>>>>>>  define all_sources
>>>>>>      ( find include -type f -name '*.h' -print; \
>>>>>>        find $(SUBDIRS) -type f -name '*.[chS]' -print )
>>>>>
>>>>> As long as it's arch/$(TARGET_ARCH) that's used here, crypto should imo
>>>>> also only be included when selected (or at the very least only when an
>>>>> arch might select it, which afaics is possible on x86 only right now).
>>>>>
>>>>> It would also help if in the description you made explicit that SUBDIRS
>>>>> isn't used for anything else (the name, after all, is pretty generic).
>>>>> Which actually points at an issue: I suppose the variable would actually
>>>>> better be used elsewhere as well, e.g. in the _clean: rule and perhaps
>>>>> also in the setting of ALL_OBJS-y. (That'll require splitting the
>>>>> variable, to that e.g. _clean would use $(SUBDIRS), $(SUBDIRS-y), and
>>>>> $(SUBDIRS-) collectively.) It is, imo, that lack of consolidation which
>>>>> has caused crypto to be missing from SUBDIRS.
>>>>>
>>>> I think what you wrote can be split into 2 parts: the part being a goal of 
>>>> this patch
>>>> and the cleanup/improvements that would be beneficial but not related to 
>>>> this patch.
>>>> The second part involves more code and there are parts to be discussed:
>>>>
>>>> 1) If we decide to create ALL_OBJS-y from SUBDIRS, then we would need to 
>>>> carve out test/ dir
>>>> that is not part of ALL_OBJS-y and add it to SUBDIRS later on. Also, the 
>>>> order of ALL_OBJS-y matters
>>>> for linking, so we would need to transfer the responsibility to SUBDIRS. 
>>>> The natural placement of
>>>> SUBDIRS (including SUBDIRS-y, etc.) would be right above ALL_OBJS-y. 
>>>> However, when doing clean (next point),
>>>> need-config is set to n and SUBDIRS would be empty. This means it would 
>>>> need to be defined somewhere at the
>>>> top of the Makefile (thus harder to make sure the linking order is 
>>>> correct).
>>>>
>>>> 2) If we deicide to use SUBDIRS for _clean rule, then we would need some 
>>>> foreach loop, right?
>>>> Apart from that, there are other directories that are not part of SUBDIRS 
>>>> i.e. include/, tools/.
>>>> Together with SUBDIRS variable, it would create confusion (these dirs are 
>>>> also sub-directories, so why
>>>> not having them listed in this variable?). Also, I can see that we do 
>>>> clean not only for $(TARGET_ARCH)
>>>> but for all the existing architectures.
>>>
>>> I understand that the changes would be more involved, but I disagree with
>>> your "two parts" statement: If what I've outlined was already the case,
>>> your patch would not even exist (because crypto/ would already be taken
>>> care of wherever needed).
>>>
>>>> I would prefer to stick for now to the goal of this patch which is to add 
>>>> crypto/ so that it is taken
>>>> into account for the tags/csope generation. Would the following change be 
>>>> ok for that purpose?
>>>>
>>>> diff --git a/xen/Makefile b/xen/Makefile
>>>> index 2d55bb9401f4..05bf301bd7ab 100644
>>>> --- a/xen/Makefile
>>>> +++ b/xen/Makefile
>>>> @@ -589,7 +589,9 @@ $(TARGET): outputmakefile FORCE
>>>>       $(Q)$(MAKE) $(build)=. arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
>>>>       $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
>>>> 'ALL_LIBS=$(ALL_LIBS-y)' $@
>>>>
>>>> -SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
>>>> +SUBDIRS-$(CONFIG_CRYPTO) += crypto
>>>> +SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test $(SUBDIRS-y)
>>>> +
>>>>  define all_sources
>>>>      ( find include -type f -name '*.h' -print; \
>>>>        find $(SUBDIRS) -type f -name '*.[chS]' -print )
>>>
>>> The fact that, afaict, this won't do is related to the outline I gave.
>>> You've referred yourself to need-config. Most if not all of the goals
>>> SUBDIRS is (currently) relevant for are listed in no-dot-config-targets.
>>> Hence your change above is effectively a no-op, because CONFIG_CRYPTO
>>> will simply be unset in the common case. Therefore if an easy change is
>>> wanted, the original version of it would be it. An intermediate
>>> approach might be to add crypto to SUBDIRS only when TARGET_ARCH=x86.
>>> But neither would preclude the same issue from being introduced again,
>>> when a new subdir appears.
>> I understand your concerns. However, I cannot see how your suggested changes
>> e.g. making use of SUBDIRS in ALL_OBJS and _clean would solve our issue.
>> They would reduce the repetitions (hence I called them cleanup/improvements)
>> but as we want to keep tags,cscope,clean as no-dot-config targets,
> 
> I, for one, didn't take this is a goal (perhaps for clean, but there ...
So you would suggest to make these targets (except clean) .config dependent?


> 
>> we would still not be able to use:
>> SUBDIRS-$(CONFIG_CRYPTO) += crypto
>> and use it in all_sources (because as you pointed out, it will be a no-op).
> 
> ... the problem is obvious to solve, as outlined before).
> 
>> Also, why do we care so much about guarding crypto/ if we take into account
>> so many architecture/config dependent files for tags/cscope (e.g. in drivers,
>> lib/x86, etc.)?
> 
> Good question, which I'm afraid I have to answer with asking back:
> 
>> If these are no-dot-config targets, then we should take everything, apart
>> from really big directories like arch/ ones.
> 
> Why is arch/ other than arch/$(TARGET_ARCH) to be ignored? And if it is,
> why would crypto, used by only x86, not be ignored? As always I'd prefer
> firm rules, not arbitrary and/or fuzzy boundaries.
> 
> Furthermore, considering e.g. cscope: Personally I view it as actively
> wrong to constrain it to just one arch. That'll lead to mistakes as
> soon as you want to make any cross-arch or even tree-wide change. Hence
> it would probably want treating just like clean. I can't comment on the
> various tags (case-insensitive) goals, as I don't know what they're
> about.
The useful thing about generating tags/cscope per-arch is that you can limit
the number of findings. Each symbol leads to one definition and not to multiple
ones. But this is still not ideal solution because doing this per-arch
and not per-config leads to inconsistency. Also, as you wrote, sometimes it is 
useful
to have all the symbols loaded when doing tree-wide changes. So in ideal world 
we should
have an option to use approach or another. I reckon this is why Linux
has a separate script for that which allows to set all-arch/per-arch, 
config-dependent/config-independent.

Anyway, I do not have any ready-made solution for this problem.
The only benefit of this patch would be that the symbols for crypto/ would be 
visible
in cscope/tags (TBOOT uses the crypto functions but there are no definitions 
present
which may be suprising for people using ctags).
But I can understand if you do not want to take this patch in, due to all the 
observations above.


~Michal