[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
On 08.11.2022 15:00, Luca Fancellu wrote: >> On 8 Nov 2022, at 11:48, Jan Beulich <jbeulich@xxxxxxxx> wrote: >> On 08.11.2022 11:59, Luca Fancellu wrote: >>>> On 07.11.2022 11:47, Luca Fancellu wrote: >>>>> @@ -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))) Hmm, not sure: --include isn't a standard option to grep, and we generally try to be portable. Actually -R (or -r) isn't either. It may still be okay that way if properly documented where the involved goals will work and where not. And then - why do you escape slashes in the ERE? Talking of escaping - personally I find backslash escapes harder to read / grok than quotation, so I'd like to recommend using quotes around each of the two --include (if they remain in the first place) instead of the \* construct. >>>>> + 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? Well, my problem is that I don't see how the distinction is conveyed without the stem being used. With what you say I understand I'm overlooking something, so I'd appreciate some explanation or at least a pointer. >>> 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. Hmm, yes, for $(objtree)/%.c.cppcheck: . Question is whether this was simply overlooked before. As said above such may be okay for these special goals, but this needs properly documenting then. >>>>> +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. I guess I don't understand: You have _analysis-% as the target, which I'd assume will handle _analysis-clean just as much as _analysis-abc. This may be connected to my lack of understanding as expressed further up. Or maybe I'm simply not understanding what the _analysis-% target is about in the first place, because with the analysis-build-% dependency I don't see how _analysis-clean would actually work (with the scope restriction you suggested earlier a rule for analysis-build-clean would not be found afaict). Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |