diff mbox

[01/27,v10] xen/arm: vpl011: Define common ring buffer helper functions in console.h

Message ID CACtJ1JQ8UYkQr4Js0hLDyQAs1N2SH9GE0pJaUjaoOsBnm8U8BQ@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bhupinder Thakur Sept. 25, 2017, 11:08 p.m. UTC 
Hi Jan,

Yes, after including the __STRICT_ANSI__ check the headers.chk check
passes. But I had to include string header file (after suggestion from Stefano)
for fixing the headers++.chk.

I have pasted the changes below:

Regards,
Bhupinder

On 25 September 2017 at 08:49, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 23.09.17 at 00:44, <sstabellini@kernel.org> wrote:
>> On Fri, 22 Sep 2017, Bhupinder Thakur wrote:
>>> DEFINE_XEN_FLEX_RING(xencons) defines common helper functions such as
>>> xencons_queued() to tell the current size of the ring buffer,
>>> xencons_mask() to mask off the index, which are useful helper functions.
>>> pl011 emulation code will use these helper functions.
>>>
>>> io/console.h includes io/ring.h which defines DEFINE_XEN_FLEX_RING.
>>>
>>> In console/daemon/io.c, string.h had to be included before io/console.h
>>> because ring.h uses string functions.
>>>
>>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> Unfortunately this patch breaks the build on x86. The reason is that
>> DEFINE_XEN_FLEX_RING requires C99, and the current header checks in
>> xen/include/Makefile use ANSI C.
>>
>> The only two headers to use DEFINE_XEN_FLEX_RING so far are pvcalls and
>> 9pfs that are both explicitly marked as c99 in xen/include/Makefile, see
>> PUBLIC_C99_HEADERS.
>>
>> One way to solve this problem would be to mark console.h as one of the
>> c99 headers, but I am guessing that Jan will want to keep it ANSI C.
>>
>> In that case, we could make DEFINE_XEN_FLEX_RING ANSI C, which is ugly
>> but possible. It requires turning the static inline functions in ring.h
>> into macros.
>>
>> Otherwise, we could take DEFINE_XEN_FLEX_RING(xencons) out of
>> io/console.h. We could move it to a new header file, and the new header
>> file could be C99.
>>
>> Jan, do you have other suggestions?
>
> Moving the C99 requiring parts to a separate header is certainly
> a reasonable option. Another might be to frame the additions by
> a __STRICT_ANSI__ and/or __GNUC__ and/or __STDC_VERSION__
> check (we have some examples elsewhere, but they may not be
> exact fits for the case here).
>
> Jan
>

Comments

Jan Beulich Sept. 26, 2017, 7:15 a.m. UTC | #1 
>>> On 26.09.17 at 01:08, <bhupinder.thakur@linaro.org> wrote:
> Yes, after including the __STRICT_ANSI__ check the headers.chk check
> passes. But I had to include string header file (after suggestion from 
> Stefano) for fixing the headers++.chk.

I'd like to have a more detailed explanation here - since the header
passed the check without this prereq before, I'd prefer if the
dependency was not added unconditionally.

> --- a/xen/include/public/io/console.h
> +++ b/xen/include/public/io/console.h
> @@ -40,7 +40,9 @@ struct xencons_interface {
>      XENCONS_RING_IDX out_cons, out_prod;
>  };
> 
> +#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>  DEFINE_XEN_FLEX_RING(xencons);
> +#endif

At the first glance it also looks as if the scope of this conditional
was too narrow.

Jan
Bhupinder Thakur Sept. 26, 2017, 8:16 a.m. UTC | #2 
Hi Jan,

On 26 September 2017 at 12:45, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.09.17 at 01:08, <bhupinder.thakur@linaro.org> wrote:
>> Yes, after including the __STRICT_ANSI__ check the headers.chk check
>> passes. But I had to include string header file (after suggestion from
>> Stefano) for fixing the headers++.chk.
>
> I'd like to have a more detailed explanation here - since the header
> passed the check without this prereq before, I'd prefer if the
> dependency was not added unconditionally.

The C header passed the check without the prereq addition. However,
for C++ headers since
__STRICT_ANSI__ is not defined, it tries to expand the
DEFINE_XEN_FLEX_RING macro
and looks for declarations for size_t, memcpy() etc. To satisfy that
requirement, string header
file had to included similar to what was done for pvcalls.

>
>> --- a/xen/include/public/io/console.h
>> +++ b/xen/include/public/io/console.h
>> @@ -40,7 +40,9 @@ struct xencons_interface {
>>      XENCONS_RING_IDX out_cons, out_prod;
>>  };
>>
>> +#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
>>  DEFINE_XEN_FLEX_RING(xencons);
>> +#endif
>
> At the first glance it also looks as if the scope of this conditional
> was too narrow.

Do you mean that xencons_interface should also be under ifdef?

Regards,
Bhupinder
Jan Beulich Sept. 26, 2017, 11:41 a.m. UTC | #3 
>>> On 26.09.17 at 10:16, <bhupinder.thakur@linaro.org> wrote:
> On 26 September 2017 at 12:45, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 26.09.17 at 01:08, <bhupinder.thakur@linaro.org> wrote:
>>> Yes, after including the __STRICT_ANSI__ check the headers.chk check
>>> passes. But I had to include string header file (after suggestion from
>>> Stefano) for fixing the headers++.chk.
>>
>> I'd like to have a more detailed explanation here - since the header
>> passed the check without this prereq before, I'd prefer if the
>> dependency was not added unconditionally.
> 
> The C header passed the check without the prereq addition. However,
> for C++ headers since
> __STRICT_ANSI__ is not defined, it tries to expand the
> DEFINE_XEN_FLEX_RING macro
> and looks for declarations for size_t, memcpy() etc. To satisfy that
> requirement, string header
> file had to included similar to what was done for pvcalls.

Ah, yes. This should equally apply to the C99 check then.

Jan
diff mbox

Patch

diff --git a/xen/include/Makefile b/xen/include/Makefile
index 1299b19..c90fdee 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -98,6 +98,7 @@  PUBLIC_C99_HEADERS := public/io/9pfs.h public/io/pvcalls.h
 PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/%
public/%hvm/save.h $(PUBLIC_C99_HEADERS), $(PUBLIC_HEADERS))

 public/io/9pfs.h-prereq := string
+public/io/console.h-prereq := string
 public/io/pvcalls.h-prereq := string

 headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
diff --git a/xen/include/public/io/console.h b/xen/include/public/io/console.h
index 5e45e1c..0f0711f 100644
--- a/xen/include/public/io/console.h
+++ b/xen/include/public/io/console.h
@@ -40,7 +40,9 @@  struct xencons_interface {
     XENCONS_RING_IDX out_cons, out_prod;
 };

+#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
 DEFINE_XEN_FLEX_RING(xencons);
+#endif

 #endif /* __XEN_PUBLIC_IO_CONSOLE_H__ */