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.
>>> @@ -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.
>> 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).
>>> + 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?
> 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).
>>> +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.
Jan
