[PATCH v2] tools: convert bitfields to unsigned type

[PATCH v2] tools: convert bitfields to unsigned type

[Olaf Hering]

clang complains about the signed type:

implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]

The potential ABI change in libxenvchan is covered by the Xen version based SONAME.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
v2: cover one more case in xenalyze

 tools/include/libxenvchan.h | 6 +++---
 tools/xentrace/xenalyze.c   | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/include/libxenvchan.h b/tools/include/libxenvchan.h
index 30cc73cf97..3d3b8aa8dd 100644
--- a/tools/include/libxenvchan.h
+++ b/tools/include/libxenvchan.h
@@ -79,11 +79,11 @@ struct libxenvchan {
 	xenevtchn_handle *event;
 	uint32_t event_port;
 	/* informative flags: are we acting as server? */
-	int is_server:1;
+	unsigned int is_server:1;
 	/* true if server remains active when client closes (allows reconnection) */
-	int server_persist:1;
+	unsigned int server_persist:1;
 	/* true if operations should block instead of returning 0 */
-	int blocking:1;
+	unsigned int blocking:1;
 	/* communication rings */
 	struct libxenvchan_ring read, write;
 	/**
diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 12dcca9646..a50538e9a8 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -1377,7 +1377,7 @@ struct hvm_data {
     tsc_t exit_tsc, arc_cycles, entry_tsc;
     unsigned long long rip;
     unsigned exit_reason, event_handler;
-    int short_summary_done:1, prealloc_unpin:1, wrmap_bf:1;
+    unsigned int short_summary_done:1, prealloc_unpin:1, wrmap_bf:1;
 
     /* Immediate processing */
     void *d;
@@ -8235,13 +8235,13 @@ void mem_set_p2m_entry_process(struct pcpu_info *p)
 
     struct {
         uint64_t gfn, mfn;
-        int p2mt;
-        int d:16,order:16;
+        uint32_t p2mt;
+        uint16_t d, order;
     } *r = (typeof(r))ri->d;
 
     if ( opt.dump_all )
     {
-        printf(" %s set_p2m_entry d%d o%d t %d g %llx m %llx\n",
+        printf(" %s set_p2m_entry d%u o%u t %u g %llx m %llx\n",
                ri->dump_header,
                r->d, r->order,
                r->p2mt,

Re: [PATCH v2] tools: convert bitfields to unsigned type

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 423 bytes --]

On 08.05.23 18:46, Olaf Hering wrote:
> clang complains about the signed type:
> 
> implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
> 
> The potential ABI change in libxenvchan is covered by the Xen version based SONAME.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v2] tools: convert bitfields to unsigned type

[Anthony PERARD]

On Tue, May 09, 2023 at 09:10:04AM +0200, Juergen Gross wrote:
> On 08.05.23 18:46, Olaf Hering wrote:
> > clang complains about the signed type:
> > 
> > implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
> > 
> > The potential ABI change in libxenvchan is covered by the Xen version based SONAME.
> > 
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Backport request (was: [PATCH v2] tools: convert bitfields to unsigned type)

[Roger Pau Monné]

On Mon, May 08, 2023 at 04:46:18PM +0000, Olaf Hering wrote:
> clang complains about the signed type:
> 
> implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
> 
> The potential ABI change in libxenvchan is covered by the Xen version based SONAME.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Can we have this one backported to 4.17 at least?

Thanks, Roger.

Re: Backport request (was: [PATCH v2] tools: convert bitfields to unsigned type)

[Jan Beulich]

On 28.06.2023 11:46, Roger Pau Monné wrote:
> On Mon, May 08, 2023 at 04:46:18PM +0000, Olaf Hering wrote:
>> clang complains about the signed type:
>>
>> implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
>>
>> The potential ABI change in libxenvchan is covered by the Xen version based SONAME.
>>
>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> Can we have this one backported to 4.17 at least?

Hmm, while perhaps simple enough, in principle this wouldn't be a backporting
candidate. May I ask why you consider this relevant? Plus is the mentioned
"potential ABI change" safe to take on a stable branch? There's not going to
be any SONAME change ...

Jan

Re: Backport request (was: [PATCH v2] tools: convert bitfields to unsigned type)

[Roger Pau Monné]

On Tue, Jul 04, 2023 at 05:42:33PM +0200, Jan Beulich wrote:
> On 28.06.2023 11:46, Roger Pau Monné wrote:
> > On Mon, May 08, 2023 at 04:46:18PM +0000, Olaf Hering wrote:
> >> clang complains about the signed type:
> >>
> >> implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
> >>
> >> The potential ABI change in libxenvchan is covered by the Xen version based SONAME.
> >>
> >> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> > 
> > Can we have this one backported to 4.17 at least?
> 
> Hmm, while perhaps simple enough, in principle this wouldn't be a backporting
> candidate. May I ask why you consider this relevant?

I have to take this fix in order to build 4.17 with current FreeBSD
clang.  I think in the past we have backported changes in order to
build with newer gcc versions.

> Plus is the mentioned
> "potential ABI change" safe to take on a stable branch? There's not going to
> be any SONAME change ...

Is there any ABI change in practice? Both fields will still have a 1bit
size.

Thanks, Roger.

Re: Backport request (was: [PATCH v2] tools: convert bitfields to unsigned type)

[Jan Beulich]

On 04.07.2023 17:55, Roger Pau Monné wrote:
> On Tue, Jul 04, 2023 at 05:42:33PM +0200, Jan Beulich wrote:
>> On 28.06.2023 11:46, Roger Pau Monné wrote:
>>> On Mon, May 08, 2023 at 04:46:18PM +0000, Olaf Hering wrote:
>>>> clang complains about the signed type:
>>>>
>>>> implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
>>>>
>>>> The potential ABI change in libxenvchan is covered by the Xen version based SONAME.
>>>>
>>>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>>>
>>> Can we have this one backported to 4.17 at least?
>>
>> Hmm, while perhaps simple enough, in principle this wouldn't be a backporting
>> candidate. May I ask why you consider this relevant?
> 
> I have to take this fix in order to build 4.17 with current FreeBSD
> clang.  I think in the past we have backported changes in order to
> build with newer gcc versions.

We did, and this is good enough a justification.

>> Plus is the mentioned
>> "potential ABI change" safe to take on a stable branch? There's not going to
>> be any SONAME change ...
> 
> Is there any ABI change in practice? Both fields will still have a 1bit
> size.

But what a consumer of the interface reads out of such a field would change
in case their compiler settings arrange for signed bitfields when signedness
isn't explicit. We don't dictate, after all, what compiler settings to use
with our interfaces (which generally is good, but which bites us here).

Jan

Re: Backport request (was: [PATCH v2] tools: convert bitfields to unsigned type)

[Roger Pau Monné]

On Tue, Jul 04, 2023 at 06:04:36PM +0200, Jan Beulich wrote:
> On 04.07.2023 17:55, Roger Pau Monné wrote:
> > On Tue, Jul 04, 2023 at 05:42:33PM +0200, Jan Beulich wrote:
> >> On 28.06.2023 11:46, Roger Pau Monné wrote:
> >>> On Mon, May 08, 2023 at 04:46:18PM +0000, Olaf Hering wrote:
> >>>> clang complains about the signed type:
> >>>>
> >>>> implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Wsingle-bit-bitfield-constant-conversion]
> >>>>
> >>>> The potential ABI change in libxenvchan is covered by the Xen version based SONAME.
> >>>>
> >>>> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> >>>
> >>> Can we have this one backported to 4.17 at least?
> >>
> >> Hmm, while perhaps simple enough, in principle this wouldn't be a backporting
> >> candidate. May I ask why you consider this relevant?
> > 
> > I have to take this fix in order to build 4.17 with current FreeBSD
> > clang.  I think in the past we have backported changes in order to
> > build with newer gcc versions.
> 
> We did, and this is good enough a justification.
> 
> >> Plus is the mentioned
> >> "potential ABI change" safe to take on a stable branch? There's not going to
> >> be any SONAME change ...
> > 
> > Is there any ABI change in practice? Both fields will still have a 1bit
> > size.
> 
> But what a consumer of the interface reads out of such a field would change
> in case their compiler settings arrange for signed bitfields when signedness
> isn't explicit. We don't dictate, after all, what compiler settings to use
> with our interfaces (which generally is good, but which bites us here).

Hm, I see.  I would argue that sign doesn't matter here, as those are
intended to be booleans, so anything different than 0 would map to
`true`.  But implementation might have hard coded TRUE to -1, and the
change would then break them?

I'm failing to see that, because those implementations would still use
the old struct declarations they have been built with, and hence would
still threat it as signed?

Thanks, Roger.

Re: Backport request (was: [PATCH v2] tools: convert bitfields to unsigned type)

[Jan Beulich]

On 04.07.2023 18:10, Roger Pau Monné wrote:
> On Tue, Jul 04, 2023 at 06:04:36PM +0200, Jan Beulich wrote:
>> On 04.07.2023 17:55, Roger Pau Monné wrote:
>>> On Tue, Jul 04, 2023 at 05:42:33PM +0200, Jan Beulich wrote:
>>>> Plus is the mentioned
>>>> "potential ABI change" safe to take on a stable branch? There's not going to
>>>> be any SONAME change ...
>>>
>>> Is there any ABI change in practice? Both fields will still have a 1bit
>>> size.
>>
>> But what a consumer of the interface reads out of such a field would change
>> in case their compiler settings arrange for signed bitfields when signedness
>> isn't explicit. We don't dictate, after all, what compiler settings to use
>> with our interfaces (which generally is good, but which bites us here).
> 
> Hm, I see.  I would argue that sign doesn't matter here, as those are
> intended to be booleans, so anything different than 0 would map to
> `true`.  But implementation might have hard coded TRUE to -1, and the
> change would then break them?

That's a possible scenario I'm wary of here, yes.

> I'm failing to see that, because those implementations would still use
> the old struct declarations they have been built with, and hence would
> still threat it as signed?

Until they rebuild against the updated header, without any change to
their code.

Anthony - do you have any thoughts here?

Jan

Re: Backport request (was: [PATCH v2] tools: convert bitfields to unsigned type)

[Jan Beulich]

On 04.07.2023 18:16, Jan Beulich wrote:
> On 04.07.2023 18:10, Roger Pau Monné wrote:
>> On Tue, Jul 04, 2023 at 06:04:36PM +0200, Jan Beulich wrote:
>>> On 04.07.2023 17:55, Roger Pau Monné wrote:
>>>> On Tue, Jul 04, 2023 at 05:42:33PM +0200, Jan Beulich wrote:
>>>>> Plus is the mentioned
>>>>> "potential ABI change" safe to take on a stable branch? There's not going to
>>>>> be any SONAME change ...
>>>>
>>>> Is there any ABI change in practice? Both fields will still have a 1bit
>>>> size.
>>>
>>> But what a consumer of the interface reads out of such a field would change
>>> in case their compiler settings arrange for signed bitfields when signedness
>>> isn't explicit. We don't dictate, after all, what compiler settings to use
>>> with our interfaces (which generally is good, but which bites us here).
>>
>> Hm, I see.  I would argue that sign doesn't matter here, as those are
>> intended to be booleans, so anything different than 0 would map to
>> `true`.  But implementation might have hard coded TRUE to -1, and the
>> change would then break them?
> 
> That's a possible scenario I'm wary of here, yes.
> 
>> I'm failing to see that, because those implementations would still use
>> the old struct declarations they have been built with, and hence would
>> still threat it as signed?
> 
> Until they rebuild against the updated header, without any change to
> their code.
> 
> Anthony - do you have any thoughts here?

Btw in the meantime I'll queue the uncontroversial part of the patch
for backport (with a respective not about what was dropped).

Jan