Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
Willy, On 10/18/21 10:51 AM, Willy Tarreau wrote: On Mon, Oct 18, 2021 at 09:18:12AM +0200, Tim Düsterhus wrote: Hu, interesting. Is the GitHub Mirror Sync broken? I'm seeing changes in https://git.haproxy.org/?p=haproxy.git, but not in GitHub. So it was in relation with the Painful Access Token apparently. The mirror user was not allowed anymore to push to .github/workflows: $ git push github Counting objects: 99, done. Delta compression using up to 2 threads. Compressing objects: 100% (63/63), done. Writing objects: 100% (63/63), 6.69 KiB | 0 bytes/s, done. Total 63 (delta 51), reused 0 (delta 0) remote: Resolving deltas: 100% (51/51), completed with 34 local objects. To https://github.com/haproxy/haproxy.git ! [remote rejected] master -> master (refusing to allow a Personal Access Token to create or update workflow `.github/workflows/codespell.yml` without `workflow` scope) I don't really see the relation with any of the recent changes. Thus I switched to SSH and got rid of the stupid clear-text PAT and now it's OK again. GitHub Action Workflows are pretty powerful and can do all kinds of stuff within a repository. I assume that GitHub wanted to increase the security by not allowing arbitrary tokens to change them, when the purpose of the token is something entirely different. I assume that simply using SSH will be stable going forward. That's all I ever used for write access since I signed up to GitHub. Best regards Tim Düsterhus
Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
On Mon, Oct 18, 2021 at 09:18:12AM +0200, Tim Düsterhus wrote: > Hu, interesting. Is the GitHub Mirror Sync broken? I'm seeing changes in > https://git.haproxy.org/?p=haproxy.git, but not in GitHub. So it was in relation with the Painful Access Token apparently. The mirror user was not allowed anymore to push to .github/workflows: $ git push github Counting objects: 99, done. Delta compression using up to 2 threads. Compressing objects: 100% (63/63), done. Writing objects: 100% (63/63), 6.69 KiB | 0 bytes/s, done. Total 63 (delta 51), reused 0 (delta 0) remote: Resolving deltas: 100% (51/51), completed with 34 local objects. To https://github.com/haproxy/haproxy.git ! [remote rejected] master -> master (refusing to allow a Personal Access Token to create or update workflow `.github/workflows/codespell.yml` without `workflow` scope) I don't really see the relation with any of the recent changes. Thus I switched to SSH and got rid of the stupid clear-text PAT and now it's OK again. Thanks, Willy
Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
On Mon, Oct 18, 2021 at 09:18:12AM +0200, Tim Düsterhus wrote: > Willy, > > On 10/18/21 9:15 AM, Willy Tarreau wrote: > > On Mon, Oct 18, 2021 at 09:09:01AM +0200, Tim Düsterhus wrote: > > > Feel free to replace 'unsigned int' with 'uint' and reformat the struct as > > > needed. > > > > Done an pushed, thank you! > > Willy > > > > Hu, interesting. Is the GitHub Mirror Sync broken? I'm seeing changes in > https://git.haproxy.org/?p=haproxy.git, but not in GitHub. Indeed, it seems that nothing was updated since dev10 this week-end. I'll have a look. Thanks for reporting it! Willy
Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
Willy, On 10/18/21 9:15 AM, Willy Tarreau wrote: On Mon, Oct 18, 2021 at 09:09:01AM +0200, Tim Düsterhus wrote: Feel free to replace 'unsigned int' with 'uint' and reformat the struct as needed. Done an pushed, thank you! Willy Hu, interesting. Is the GitHub Mirror Sync broken? I'm seeing changes in https://git.haproxy.org/?p=haproxy.git, but not in GitHub. Best regards Tim Düsterhus
Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
On Mon, Oct 18, 2021 at 09:09:01AM +0200, Tim Düsterhus wrote: > Feel free to replace 'unsigned int' with 'uint' and reformat the struct as > needed. Done an pushed, thank you! Willy
Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
Willy,
On 10/18/21 7:22 AM, Willy Tarreau wrote:
On Sat, Oct 16, 2021 at 06:24:18PM +0200, Tim Duesterhus wrote:
see 6a0dd733906611dea958cf74b9f51bb16028ae20
Found using GitHub's CodeQL scan.
---
include/haproxy/stick_table-t.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/haproxy/stick_table-t.h b/include/haproxy/stick_table-t.h
index 3b1f2b3ef..133f992b5 100644
--- a/include/haproxy/stick_table-t.h
+++ b/include/haproxy/stick_table-t.h
@@ -125,8 +125,8 @@ struct stktable_data_type {
const char *name; /* name of the data type */
int std_type; /* standard type we can use for this data, STD_T_* */
int arg_type; /* type of optional argument, ARG_T_* */
- int is_array:1; /* this is an array of gpc/gpt */
- int is_local:1; /* this is local only and never learned */
+ unsigned int is_array:1; /* this is an array of gpc/gpt */
+ unsigned int is_local:1; /* this is local only and never learned */
Please note, since last patch we've added shorter type names like "uint"
for the unsigned int, that help preserve alignment. Do you mind if I adapt
your patch ?
Feel free to replace 'unsigned int' with 'uint' and reformat the struct
as needed.
Best regards
Tim Düsterhus
Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
On Sat, Oct 16, 2021 at 06:24:18PM +0200, Tim Duesterhus wrote:
> see 6a0dd733906611dea958cf74b9f51bb16028ae20
>
> Found using GitHub's CodeQL scan.
> ---
> include/haproxy/stick_table-t.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/haproxy/stick_table-t.h b/include/haproxy/stick_table-t.h
> index 3b1f2b3ef..133f992b5 100644
> --- a/include/haproxy/stick_table-t.h
> +++ b/include/haproxy/stick_table-t.h
> @@ -125,8 +125,8 @@ struct stktable_data_type {
> const char *name; /* name of the data type */
> int std_type; /* standard type we can use for this data, STD_T_* */
> int arg_type; /* type of optional argument, ARG_T_* */
> - int is_array:1; /* this is an array of gpc/gpt */
> - int is_local:1; /* this is local only and never learned */
> + unsigned int is_array:1; /* this is an array of gpc/gpt */
> + unsigned int is_local:1; /* this is local only and never learned */
Please note, since last patch we've added shorter type names like "uint"
for the unsigned int, that help preserve alignment. Do you mind if I adapt
your patch ?
Willy
Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
On Sat, Jan 18, 2020 at 11:41:58AM +0100, Tim Düsterhus wrote: > Fair enough. In the specific implementation of gcc it might be okay. But > that doesn't say anything about clang (but I guess it's okay as well). Clang tries hard to support whatever gcc does and when I mean gcc I implicitly also mean clang (and very likely icc as well for those who care). > > For this reason I'd like to get the signed-vs-unsigned part merged. > > Feel free to edit around in my patch to remove the bits that change from > something to `int` and leave the adding of the `unsigned` in there. Or > drop my patch, create your own one and add me as Co-authored-by or > Reported-by or whatever. OK so I simply edited your patch. > While doing so please check on the `struct pat_time` I mentioned in my > previous email. Ah yes, good point. I've just checked, I added this in 2007 and never implemented the time range parser. I we managed not to need if for the last 13 years, I think we don't need it and the day we need it we might do it differently, so I'm simply removing that definition. thanks! Willy
Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
Willy, Am 18.01.20 um 07:51 schrieb Willy Tarreau: >> - Non-ints are not allowed by the C standard. > > Hmmm that's not exactly what I'm reading in C99 #6.7.2.1 here, which > explicitly permits implementation-defined types: > >"A bit-field shall have a type that is a qualified or unqualified version > of >_Bool, signed int, unsigned int, or some other implementation-defined > type." > > And the implementation we rely on (gcc) says: > > > https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html > Allowable bit-field types other than _Bool, signed int, and unsigned int > (C99 and C11 6.7.2.1). Other integer types, such as long int, and > enumerated types are permitted even in strictly conforming mode. Fair enough. In the specific implementation of gcc it might be okay. But that doesn't say anything about clang (but I guess it's okay as well). > And this is not surprizing for both tools, because I think that with the > change, the unsigned int field is merged with the previous chars, which > is particularly dirty. And if we place the field before the chars, it > leaves a two-bytes hole between them. > > Ideally I'd use flags there, though we'd have to place them in an unsigned > char. If you're interested in having a look at such a change I'd prefer it, > otherwise I'd prefer not to change what exists for this part because it does > exactly what we need and the change makes it wrong at least debug-wise. I'm not interested. The change to int from char was just a by-product of the signedness changes I made. As you might imagine I was working off the output of some static analyzer which complained that the size `1` bitfield might not be what was meant. While I heard about the existence of bitfields before I have no idea how they worked, read a bit about them and then changed them all to be consistently `int`, because that's what's guaranteed by the standard. Long story short: If you prefer these to remain chars, then so be it. > For this reason I'd like to get the signed-vs-unsigned part merged. Feel free to edit around in my patch to remove the bits that change from something to `int` and leave the adding of the `unsigned` in there. Or drop my patch, create your own one and add me as Co-authored-by or Reported-by or whatever. While doing so please check on the `struct pat_time` I mentioned in my previous email. > Did you check whether this produced any output code change ? It would > be a hint that we might possibly have latent bugs. No, because this is way outside the area of my expertise :-) Best regards Tim Düsterhus
Re: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
Hi Tim,
On Sat, Jan 18, 2020 at 01:32:49AM +0100, Tim Duesterhus wrote:
> Willy,
>
> I did not touch the `struct pat_time` in pattern.h which appears to be
> completely unused. Please check whether adjustments should be made there
> as well or whether it should simply be removed. Feel free to amend my
> patch if necessary.
>
> Best regards
> Tim Düsterhus
>
> Apply with `git am --scissors` to automatically cut the commit message.
>
> -- >8 --
> Subject: [PATCH] CLEANUP: Consistently `unsigned int` for bitfields
>
> - Non-ints are not allowed by the C standard.
Hmmm that's not exactly what I'm reading in C99 #6.7.2.1 here, which
explicitly permits implementation-defined types:
"A bit-field shall have a type that is a qualified or unqualified version of
_Bool, signed int, unsigned int, or some other implementation-defined type."
And the implementation we rely on (gcc) says:
https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html
Allowable bit-field types other than _Bool, signed int, and unsigned int
(C99 and C11 6.7.2.1). Other integer types, such as long int, and
enumerated types are permitted even in strictly conforming mode.
I tend to find it extremely confusing to place "unsigned int" just after a
char, because it makes one think we're leaving a 2-byte hole before and/or
that we can use up to 32 bits without causing misalignment while in practice
we can only use the few bits left from the missing bytes. That's what we
have in fdtab:
void *owner;
unsigned char state; <-- 8-aligned
unsigned char ev; <-- 8-aligned + 1
unsigned char linger_risk:1<-- 8-aligned + 2;
unsigned char cloned:1;
unsigned char initialized:1;
With the change it remains the same except that it looks quite non-obvious
as the hole will depend on the number of bits added:
void *owner;
unsigned char state; <-- 8-aligned
unsigned char ev; <-- 8-aligned + 1
unsigned int linger_risk:1 <-- 8-aligned + 2;
unsigned int cloned:1;
unsigned int initialized:1;
Interestingly, the change above confuses pahole:
struct fdtab {
long unsigned int lock; /* 0 8 */
long unsigned int thread_mask; /* 8 8 */
long unsigned int update_mask; /*16 8 */
struct fdlist_entryupdate; /*24 8 */
void (*iocb)(int); /*32 8 */
void * owner;/*40 8 */
unsigned char state;/*48 1 */
unsigned char ev; /*49 1 */
/* Bitfield combined with previous fields */
unsigned int linger_risk:1;/*48:15 4 */
unsigned int cloned:1; /*48:14 4 */
unsigned int initialized:1;/*48:13 4 */
^^^
Here it expects to reserve 4 bytes for the field
/* size: 56, cachelines: 1, members: 11 */
/* padding: 4 */
/* bit_padding: 29 bits */
/* last cacheline: 56 bytes */
/* BRAIN FART ALERT! 56 != 56 + 0(holes), diff = 0 */
^^
And this surprizing warning (certainly a bug). Without this change, the
output looks more consistent:
unsigned char state;/*48 1 */
unsigned char ev; /*49 1 */
unsigned char linger_risk:1;/*50: 7 1 */
unsigned char cloned:1; /*50: 6 1 */
unsigned char initialized:1;/*50: 5 1 */
More annoyingly it confuses gdb:
Before:
(gdb) p [0].linger_risk
$4 = (unsigned char *) 0x32
After:
(gdb) p [0].linger_risk
$1 = (unsigned int *) 0x30
And this is not surprizing for both tools, because I think that with the
change, the unsigned int field is merged with the previous chars, which
is particularly dirty. And if we place the field before the chars, it
leaves a two-bytes hole between them.
Ideally I'd use flags there, though we'd have to place them in an unsigned
char. If you're interested in having a look at such a change I'd prefer it,
otherwise I'd prefer not to change what exists for this part because it does
exactly what we need and the change makes it wrong at least debug-wise.
> - Signed bitfields of size `1` hold the values `0` and `-1`, but are
> usually assigned `1`, possibly leading to subtle bugs when the value
> is explicitely compared against `1`.
Excellent point! The problem is even more subtle when
