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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2023 09:14, Michal Orzel wrote:
> 
> 
> On 27/02/2023 16:57, Jan Beulich wrote:
>>
>>
>> On 27.02.2023 16:46, Michal Orzel wrote:
>>> 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.
>>
>> Well, I'm not outright opposed. But I definitely want to hear another
>> maintainer's view before deciding.
> 
> While waiting for other maintainers vote, I would like to propose an 
> intermediate approach
> that would at least result in unified approach (related to what you wrote 
> about constraints):
> 
> In general, there are 2 *main* approaches when dealing with code indexing.
> The first is a "tree-wide" approach, where we do not take Kconfig into 
> account.
> The second is a "config-aware" approach, where we take into account Kconfig 
> options.
> 
> At the moment, in Xen, the approach taken is not uniform because we take all 
> the directories
> into account except arch/ where we take only $(TARGET_ARCH) into indexing. 
> This makes it difficult
> to reason about. What is the rule then - how big is the directory?
> 
> The intermediate solution is to switch for now to "tree-wide" approach by 
> modifying the SUBDIRS
> from:
> SUBDIRS = xsm arch/$(TARGET_ARCH) common drivers lib test
> to:
> SUBDIRS = xsm arch common drivers lib test crypto
> 
> This way, the approach is clear for everyone and we do not make any 
> exceptions. Also the name of
> SUBDIRS and all_sources makes sense as they are read as "global" by default 
> as oppose to e.g. "all_compiled_sources".
> In the future, we can then add support for "config-aware" approach by taking 
> into account Kconfig which involes
> more code.
> 
> Benefits:
>  - clear approach, no fuzzy boundaries - all in
>  - crypto can be included right away as well as any new subdir without 
> requiring to fix the infrastructure first

I'm okay with that proposal (albeit if you make a patch, please have "crypto"
at least ahead of "test"), but it'll need agreement by people actually using
any of the affected make goals.

Jan