> On 8 Nov 2022, at 11:48, Jan Beulich <jbeul...@suse.com> wrote:
>
> On 08.11.2022 11:59, Luca Fancellu wrote:
>>> On 07.11.2022 11:47, Luca Fancellu wrote:
>>>> +Here is an example to add a new justification in
>>>> false-positive-<tool>.json::
>>>
>>> With <tool> already present in the name, ...
>>>
>>>> +|{
>>>> +| "version": "1.0",
>>>> +| "content": [
>>>> +| {
>>>> +| "id": "SAF-0-false-positive-<tool>",
>>>> +| "analyser": {
>>>> +| "<tool>": "<proprietary-id>"
>>>
>>> ... can we avoid the redundancy here? Perhaps ...
>>>
>>>> +| },
>>>> +| "tool-version": "<version>",
>>>
>>> ... it could be
>>>
>>> "analyser": {
>>> "<version>": "<proprietary-id>"
>>> },
>>
>> Yes it’s a bit redundant but it helps re-using the same tool we use for
>> safe.json
>
> I guess the tool could also be made cope without much effort.
I can modify the script to take an additional parameter to distinguish between
safe.json
and false-positive-*.json, then call twice the script and append the result to
the .sed file.
>
>>>> @@ -757,6 +758,51 @@ cppcheck-version:
>>>> $(objtree)/include/generated/compiler-def.h:
>>>> $(Q)$(CC) -dM -E -o $@ - < /dev/null
>>>>
>>>> +JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>>>> + $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>>> +
>>>> +# The following command is using grep to find all files that contains a
>>>> comment
>>>> +# containing "SAF-<anything>" on a single line.
>>>> +# %.safparse will be the original files saved from the build system,
>>>> these files
>>>> +# will be restored at the end of the analysis step
>>>> +PARSE_FILE_LIST := $(addsuffix .safparse,$(filter-out %.safparse,\
>>>> +$(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$'
>>>> $(srctree))))
>>>
>>> Please indent such line continuations. And then isn't this going to risk
>>> matching non-source files as well? Perhaps you want to restrict this to
>>> *.c and *.h?
>>
>> Yes, how about this, it will filter out *.safparse files while keeping in
>> only .h and .c:
>>
>> PARSE_FILE_LIST := $(addsuffix .safparse,$(filter %.c %.h,\
>> $(shell grep -ERl '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$'
>> $(srctree))))
>
> That's better, but still means touching all files by grep despite now
> only a subset really looked for. If I was to use the new goals on a
> more or less regular basis, I'd expect that this enumeration of files
> doesn't read _much_ more stuff from disk than is actually necessary.
Ok would it be ok?
PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include=\*.h \
--include=\*.c '^[[:blank:]]*\/\*[[:space:]]+SAF-.*\*\/$$' $(srctree)))
>
>>> To limit work done, could this me "mv" instead of "cp -p", and then ...
>>>
>>>> +analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
>>>> + $(Q)for file in $(patsubst %.safparse,%,$(PARSE_FILE_LIST)); do \
>>>> + sed -i -f "$(objtree)/$*.sed" "$${file}"; \
>>>
>>> ... with then using
>>>
>>> sed -f "$(objtree)/$*.sed" "$${file}.safparse" >"$${file}"
>>>
>>> here? This would then also have source consistent between prereqs and
>>> rule.
>>
>> We saw that mv is not preserving the timestamp of the file, instead we would
>> like to preserve
>> it, for this reason we used cp -p
>
> Buggy mv? It certainly doesn't alter timestamps here, and I don't think
> the spec allows for it doing so (at least when it doesn't need to resort
> to copying to deal with cross-volume moves, but those can't happen here).
Yes you are right, my assumption was wrong, I will change the code as you
suggested.
>
>>>> + done
>>>> +
>>>> +analysis-build-%: analysis-parse-tags-%
>>>> + $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>>>
>>> This rule doesn't use the stem, so I'm struggling to understand what
>>> this is about.
>>
>> Yes, here my aim was to catch analysis-build-{eclair,coverity}, here I see
>> that if the user has a typo
>> the rule will run anyway, but it will be stopped by the dependency chain
>> because at the end we have:
>>
>> JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
>> $(XEN_ROOT)/docs/misra/false-positive-$$*.json
>>
>> That will give an error because
>> $(XEN_ROOT)/docs/misra/false-positive-<typo>.json does not exists.
>>
>> If you think it is not enough, what if I reduce the scope of the rule like
>> this?
>>
>> _analysis-coverity _analysis-eclair: _analysis-%: analysis-build-%
>
> But then, without using the stem, how does it know whether to do an
> Eclair or a Coverity run?
Sorry I think I’m a bit lost here, the makefile is working on both
analysis-coverity and analysis-eclair
because the % is solving in coverity or eclair depending on which the makefile
has in input, it is not complaining
so I guess it works.
Do you see something not working? If so, are you able to provide a piece of
code for that to make me understand?
>
>> Or, if you are still worried about “analysis-build-%:
>> analysis-parse-tags-%”, then I can do something
>> like this:
>>
>> analysis-supported-coverity analysis-supported-eclair:
>> @echo > /dev/null
>>
>> analysis-supported-%:
>> @error Unsupported analysis tool @*
>>
>> analysis-build-%: analysis-parse-tags-% | analysis-supported-%
>> $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build
>
> If I'm not mistaken support for | doesn't exist in make 3.80 (the
> minimum version we require to be used).
IDK, we use order-only prerequisite already in the Makefile.
>
>>>> +analysis-clean:
>>>> +# Reverts the original file (-p preserves also timestamp)
>>>> + $(Q)find $(srctree) -type f -name "*.safparse" -print | \
>>>> + while IFS= read file; do \
>>>> + cp -p "$${file}" "$${file%.safparse}"; \
>>>> + rm -f "$${file}"; \
>>>
>>> Why not "mv"?
>>>
>>>> + done
>>>> +
>>>> +_analysis-%: analysis-build-%
>>>> + $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean
>>>
>>> Again no use of the stem, plus here I wonder if this may not lead to
>>> people invoking "analysis-clean" without having said anything about
>>> cleaning on their command line.
>>
>> In any case, the cleaning process is very safe and does not clean anything
>> that was not dirty before,
>> so in case of typos, it’s just like a nop.
>
> People may put transient files in their trees. Of course they need to be
> aware that when they specify a "clean" target their files may be deleted.
> But without any "clean" target specified nothing should be removed.
*.safparse files are not supposed to be used freely by user in their tree, those
files will be removed only if the user calls the “analysis-clean” target or if
the
analysis-coverity or analysis-eclair reaches the end (a process that creates
*.safparse).
There is no other way to trigger the “analysis-clean” unintentionally, so I’m
not sure about
the modification you would like to see there.
>
> Jan
