Re: [PATCH v6 06/11] target/arm: Factor out code for setting MTE TCF0 field

[Gustavo Romero <gustavo.romero@linaro.org>]

Hi Richard,

On 6/28/24 2:00 PM, Richard Henderson wrote:
> On 6/28/24 08:49, Gustavo Romero wrote:
>> I thought you meant osdep.h should not be included _at all_ in my case, either
>> in mte_user_helper.h or in mte_user_helper.c. Maybe the wording in the docs
>> should be "Do not include "qemu/osdep.h" from header files. Include it from .c
>> files, when necessary.".
> 
> Not "when necessary", always, and always first.

Got it!


> See the "Include directives" section of docs/devel/style.rst, which does explicitly say 'Do not include "qemu/osdep.h" from header files'.

Yep, Phil pointed out this doc when we were discussing it in v5.
I was actually referring to it about the wording. Maybe then it should
be more explicitly that osdep.h _always_ has to be present.

Re-reading it after your clarifications makes it clear, but the first time
Phil pointed it out the phrases:

"[...] since the .c file will have already included it." and
"Headers should normally include everything they need beyond osdep.h."

weren't enough to me to make it clear that osdep.h must always be included
(present) in the .c files. "will have already included" sounded ambiguous to
me, more like, if necessary it would have already be included in .c (but not
always). But, well, that can be a falt in my interpretation..

Thanks a lot for the clarification.


> 
>> I think we agree osdep.h is necessary and must be put in mte_user_helper.c. But
>> that left me wondering how it would work for sources including mte_user_helper.h,
>> because it can be the case they don't have the declarations for the types used in
>> the function prototypes, in this case, for CPUArchState and abi_long types in
>> arm_set_mte_tcf0.
> 
> CPUArchState will come from qemu/typedefs.h via osdep.h.
> 
> For this particular function, 'int' would have been enough,
> since we only care about the low two bits.

hmm, right. I'll send a follow up patch to improve it since Alex already picked up
the series to gdbstub/next. Thanks!


Cheers,
Gustavo