All of lore.kernel.org
 help / color / mirror / Atom feed
From: Luca Fancellu <Luca.Fancellu@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Wei Chen <Wei.Chen@arm.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair
Date: Tue, 8 Nov 2022 17:13:18 +0000	[thread overview]
Message-ID: <2E0BFEFC-5BEE-4F8B-BD9E-94CB9A5B2BC9@arm.com> (raw)
In-Reply-To: <7d56c33d-4b03-9aa1-6abc-45a8ad41caca@suse.com>



> On 8 Nov 2022, at 15:49, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 08.11.2022 15:00, Luca Fancellu wrote:
>>> On 8 Nov 2022, at 11:48, Jan Beulich <jbeulich@suse.com> 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.

Is a comment before the line ok as documentation? To state that —include and
-R are not standard options so analysis-{coverity,eclair} will not work without a
grep that takes those parameters?

> 
> 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.

Ok I’ve removed the escape from the * and also from slashes:

PARSE_FILE_LIST := $(addsuffix .safparse,$(shell grep -ERl --include='*.h' \
    --include='*.c' '^[[:blank:]]*/\*[[:space:]]+SAF-.*\*/$$' $(srctree)))

> 
>>>>>> +	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.

Ok, I have that eclair and coverity shares the same commands to be executed by the build system,
so instead of duplicating the targets for coverity and eclair and their recipe, I’ve used the pattern rule
to have that these rules:

JUSTIFICATION_FILES := $(XEN_ROOT)/docs/misra/safe.json \
                       $(XEN_ROOT)/docs/misra/false-positive-$$*.json

[…]

.SECONDEXPANSION:
$(objtree)/%.sed: $(srctree)/tools/xenfusa-gen-tags.py $(JUSTIFICATION_FILES)
    […]

[…]

analysis-parse-tags-%: $(PARSE_FILE_LIST) $(objtree)/%.sed
    […]

analysis-build-%: analysis-parse-tags-%
    $(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile build

analysis-clean:
   […]

_analysis-%: analysis-build-%
    $(Q)$(MAKE) O=$(abs_objtree) -f $(srctree)/Makefile analysis-clean

Matches the case where 'make analysis-coverity’ or ‘make analysis-eclair’ is called.

Now, please correct me if my assumption on the way make works are wrong, here my assumptions:

For example when ‘make analysis-coverity’ is called we have that this rule is the best match for the
called target:

_analysis-%:

So anything after _analysis- will be captured with % and this will be transferred to the dependency
of the target that is analysis-build-% -> analysis-build-coverity

Now analysis-build-coverity will be called, the best match is analysis-build-%, so again the dependency
which is analysis-parse-tags-%, will be translated to analysis-parse-tags-coverity.

Now analysis-parse-tags-coverity will be called, the best match is analysis-parse-tags-%, so the % will
Have the ‘coverity’ value and in the dependency we will have $(objtree)/%.sed -> $(objtree)/coverity.sed.

Looking for $(objtree)/coverity.sed the best match is $(objtree)/%.sed, which will have $(JUSTIFICATION_FILES)
and the python script in the dependency, here we will use the second expansion to solve
$(XEN_ROOT)/docs/misra/false-positive-$$*.json in $(XEN_ROOT)/docs/misra/false-positive-coverity.json

So now after analysis-parse-tags-coverity has ended its dependency it will start with its recipe, after it finishes,
the recipe of analysis-build-coverity will start and it will call make to actually build Xen.

After the build finishes, if the status is good, the analysis-build-coverity has finished and the _analysis-coverity
recipe can now run, it will call make with the analysis-clean target, restoring any <file>.{c,h}.safparse to <file>.{c,h}.

We will have the same with ‘make analysis-eclair’, if we do a mistake typing, like ‘make analysis-coveri’, we will
have:

make: Entering directory ‘/path/to/xen/xen'
make: *** No rule to make target 'analysis-coveri'.  Stop.
make: Leaving directory '/path/to/xen/xen'



> 
>>>> 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).

_analysis-clean will not work, neither _analysis-abc, because of what I wrote above.
analysis-clean instead is called from the recipe of _analysis-% if all its dependency are
built correctly, otherwise it’s the user that needs to call it directly by doing “make analysis-clean”.



> 
> Jan


  reply	other threads:[~2022-11-08 17:13 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-07 10:47 [RFC PATCH 0/4] Static analyser finding deviation Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 1/4] xen/Makefile: add analysis-coverity and analysis-eclair Luca Fancellu
2022-11-07 16:35   ` Jan Beulich
2022-11-08 10:59     ` Luca Fancellu
2022-11-08 11:48       ` Jan Beulich
2022-11-08 14:00         ` Luca Fancellu
2022-11-08 15:49           ` Jan Beulich
2022-11-08 17:13             ` Luca Fancellu [this message]
2022-11-09  8:31               ` Jan Beulich
2022-11-09 10:08                 ` Luca Fancellu
2022-11-09 10:36                   ` Jan Beulich
2022-11-11 10:42                     ` Luca Fancellu
2022-11-11 13:10                       ` Jan Beulich
2022-11-11 20:52                         ` Stefano Stabellini
2022-11-14  7:30                           ` Jan Beulich
2022-11-14 12:30                             ` Luca Fancellu
2022-11-14 16:05                               ` Jan Beulich
2022-11-14 17:16                               ` Anthony PERARD
2022-11-14 16:25   ` Anthony PERARD
2022-11-25  8:50     ` Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 2/4] xen/Makefile: add analysis-cppcheck and analysis-cppcheck-html Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 3/4] tools/misra: fix skipped rule numbers Luca Fancellu
2022-11-07 10:47 ` [RFC PATCH 4/4] xen: Justify linker script defined symbols in include/xen/kernel.h Luca Fancellu
2022-11-07 11:49   ` Jan Beulich
2022-11-07 11:53     ` Luca Fancellu
2022-11-07 12:56       ` Jan Beulich
2022-11-07 19:06         ` Julien Grall
2022-11-08 11:00           ` Luca Fancellu
2022-11-08 11:32             ` Julien Grall
2022-11-08 11:55               ` Luca Fancellu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2E0BFEFC-5BEE-4F8B-BD9E-94CB9A5B2BC9@arm.com \
    --to=luca.fancellu@arm.com \
    --cc=Bertrand.Marquis@arm.com \
    --cc=Wei.Chen@arm.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=george.dunlap@citrix.com \
    --cc=jbeulich@suse.com \
    --cc=julien@xen.org \
    --cc=sstabellini@kernel.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.