Re: [PATCH 1/3] x86/p2m.h: Add include guards

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.05.2022 20:42, Roberto Bagnara wrote:
> On 17/05/22 17:38, Jan Beulich wrote:
>> On 09.05.2022 14:24, Andrew Cooper wrote:
>>> Spotted by Eclair MISRA scanner.
>>
>> I'm sorry, but what exactly was it that the scanner spotted? It was
>> actually deliberate to introduce this file without guards. I'm of
>> the general opinion that (private) headers not to be included by
>> other headers (but only by .c files) are not in need of guards. If
>> it is project-wide consensus that _all_ header files should have
>> guards, then I'll try to keep this in mind (in "x86emul: a few
>> small steps towards disintegration" for example I introduce
>> another such instance), but then it should also be put down in
>> ./CODING_STYLE.
> 
> The rationale of this rule is as follows:
> 
> - With a complex hierarchy of nested header files, it is possible
>    for a header file to be included more than once.
> 
> - This can bring to circular references of header files, which
>    can result in undefined behavior and/or be difficult to debug.

This, in particular, is known to happen in our and other source
bases despite the use of guards, hence I view this point as at
best partly related. Nevertheless I agree with and understand the
reasons for using guards _where they are needed_. I do not agree
that guards need to be there for no specific reason.

Jan

> - If multiple inclusion leads to multiple or conflicting definitions,
>    then this can result in undefined or erroneous behavior.
> 
> - Compilation and analysis time is needlessly increased.
> 
> There has been a period (which lasted until the end of the '70s
> or the beginning of the '80s, I would have to dig up to be
> more precise) when the solution was thought to be "headers
> shall not to be included by other headers but only by .c files."
> Experience then showed that, in medium to large projects,
> each .c file had to begin with a long list of #include
> directives;  such lists needed to be ordered to accommodate
> the dependencies between header files;  in some cases the
> lists were so long that:
> 
> a) it was a kind of black magic to find out the right
>     inclusion order, one that would work in any of
>     possibly many project configurations;
> b) the lists of #include directives often contained duplicates,
>     possibly because the desperate programmers where trying
>     to find the right order.
> 
> In the end, the software engineering community converged
> on the idea that guards against multiple inclusion are
> a much better alternative.
> 
> Of course there are valid reasons to deviate the rule:
> some header files might be conceived to be included
> multiple times.  A one-line configuration for ECLAIR
> will do the trick to make sure such header files are
> not reported.
> 
> Kind regards,
> 
>     Roberto
> 
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> CC: Wei Liu <wl@xxxxxxx>
>>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>> CC: Julien Grall <julien@xxxxxxx>
>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>>> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>>> ---
>>>   xen/arch/x86/mm/p2m.h | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/mm/p2m.h b/xen/arch/x86/mm/p2m.h
>>> index cc0f6766e4df..dc706b8e4799 100644
>>> --- a/xen/arch/x86/mm/p2m.h
>>> +++ b/xen/arch/x86/mm/p2m.h
>>> @@ -15,6 +15,9 @@
>>>    * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>>    */
>>>   
>>> +#ifndef __ARCH_MM_P2M_H__
>>> +#define __ARCH_MM_P2M_H__
>>> +
>>>   struct p2m_domain *p2m_init_one(struct domain *d);
>>>   void p2m_free_one(struct p2m_domain *p2m);
>>>   
>>> @@ -39,6 +42,8 @@ int ept_p2m_init(struct p2m_domain *p2m);
>>>   void ept_p2m_uninit(struct p2m_domain *p2m);
>>>   void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
>>>   
>>> +#endif /* __ARCH_MM_P2M_H__ */
>>> +
>>>   /*
>>>    * Local variables:
>>>    * mode: C
>>
>>
>