| Message ID | 20230508164618.21496-1-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] tools: convert bitfields to unsigned type | expand |
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
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,
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.
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
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.
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
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.
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
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
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,
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(-)