Message ID | 20200210174325.6566-2-alistair.francis@wdc.com |
---|---|
State | New |
Headers | show |
Series | Always use long time_t for certain syscalls | expand |
On Mon, Feb 10, 2020 at 12:50 PM Alistair Francis <alistair.francis@wdc.com> wrote: > > These functions are alpha specifc, rename them to be clear. These functions were meant to be generic. I put the file in s/u/s/l/alpha because it was only *used* on Alpha, at the time. What's stopping you from using these functions on all architectures? zw
On Mon, Feb 10, 2020 at 1:11 PM Zack Weinberg <zackw@panix.com> wrote: > > On Mon, Feb 10, 2020 at 12:50 PM Alistair Francis > <alistair.francis@wdc.com> wrote: > > > > These functions are alpha specifc, rename them to be clear. > > These functions were meant to be generic. I put the file in > s/u/s/l/alpha because it was only *used* on Alpha, at the time. > > What's stopping you from using these functions on all architectures? The Alpha functions use 32-bit versions of the timeval struct for a 64-bit architecture. For all other architectures we want to use the word size version (regardless of time_t size) for the timeval struct. That's the main difference. If Alpha didn't have this requirement we could then just remove the Alpha specific overrides after this series and use all of the new generic y2038 infrastructure. Alistair > > zw
On Mon, Feb 10, 2020 at 4:25 PM Alistair Francis <alistair23@gmail.com> wrote: > On Mon, Feb 10, 2020 at 1:11 PM Zack Weinberg <zackw@panix.com> wrote: > > > > On Mon, Feb 10, 2020 at 12:50 PM Alistair Francis > > <alistair.francis@wdc.com> wrote: > > > > > > These functions are alpha specifc, rename them to be clear. > > > > These functions were meant to be generic. I put the file in > > s/u/s/l/alpha because it was only *used* on Alpha, at the time. > > > > What's stopping you from using these functions on all architectures? > > The Alpha functions use 32-bit versions of the timeval struct for a > 64-bit architecture. For all other architectures we want to use the > word size version (regardless of time_t size) for the timeval struct. > That's the main difference. I'm sorry, I haven't been following the discussion all that closely. Why is it necessary to use a 32-bit timeval struct (with 32-bit time_t, I presume) *only* on Alpha? Is there no way we can smooth over this difference so that, as you say, > If Alpha didn't have this requirement we could then just remove the > Alpha specific overrides after this series and use all of the new > generic y2038 infrastructure. Also, even if we really are stuck with an Alpha-specific struct timeval32, I don't see why that affects the definitions of the conversion functions? zw
On 10/02/2020 18:35, Zack Weinberg wrote: > On Mon, Feb 10, 2020 at 4:25 PM Alistair Francis <alistair23@gmail.com> wrote: >> On Mon, Feb 10, 2020 at 1:11 PM Zack Weinberg <zackw@panix.com> wrote: >>> >>> On Mon, Feb 10, 2020 at 12:50 PM Alistair Francis >>> <alistair.francis@wdc.com> wrote: >>>> >>>> These functions are alpha specifc, rename them to be clear. >>> >>> These functions were meant to be generic. I put the file in >>> s/u/s/l/alpha because it was only *used* on Alpha, at the time. >>> >>> What's stopping you from using these functions on all architectures? >> >> The Alpha functions use 32-bit versions of the timeval struct for a >> 64-bit architecture. For all other architectures we want to use the >> word size version (regardless of time_t size) for the timeval struct. >> That's the main difference. > > I'm sorry, I haven't been following the discussion all that closely. > Why is it necessary to use a 32-bit timeval struct (with 32-bit > time_t, I presume) *only* on Alpha? Is there no way we can smooth > over this difference so that, as you say, Alpha is an outlier here, it is the only 64-bit architecture that did the 32 bit to 64 bit transition some time ago. All the generic code assumes that 64-bit architectures always have 64-bit time_t (__ASSUME_TIME64_SYSCALLS). And the old interface is not exported and *only* meant to be used in compat symbols, different than the current way of handling y2038 prof code (where the user might still define the ABI with _TIME_SIZE). That's why I think alpha compat code should be compartmentalized on alpha sysdep folder. > >> If Alpha didn't have this requirement we could then just remove the >> Alpha specific overrides after this series and use all of the new >> generic y2038 infrastructure. > > Also, even if we really are stuck with an Alpha-specific struct > timeval32, I don't see why that affects the definitions of the > conversion functions? > > zw >
On Mon, Feb 10, 2020 at 1:53 PM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > > > > On 10/02/2020 18:35, Zack Weinberg wrote: > > On Mon, Feb 10, 2020 at 4:25 PM Alistair Francis <alistair23@gmail.com> wrote: > >> On Mon, Feb 10, 2020 at 1:11 PM Zack Weinberg <zackw@panix.com> wrote: > >>> > >>> On Mon, Feb 10, 2020 at 12:50 PM Alistair Francis > >>> <alistair.francis@wdc.com> wrote: > >>>> > >>>> These functions are alpha specifc, rename them to be clear. > >>> > >>> These functions were meant to be generic. I put the file in > >>> s/u/s/l/alpha because it was only *used* on Alpha, at the time. > >>> > >>> What's stopping you from using these functions on all architectures? > >> > >> The Alpha functions use 32-bit versions of the timeval struct for a > >> 64-bit architecture. For all other architectures we want to use the > >> word size version (regardless of time_t size) for the timeval struct. > >> That's the main difference. > > > > I'm sorry, I haven't been following the discussion all that closely. > > Why is it necessary to use a 32-bit timeval struct (with 32-bit > > time_t, I presume) *only* on Alpha? Is there no way we can smooth > > over this difference so that, as you say, > > Alpha is an outlier here, it is the only 64-bit architecture that did > the 32 bit to 64 bit transition some time ago. All the generic code > assumes that 64-bit architectures always have 64-bit time_t > (__ASSUME_TIME64_SYSCALLS). > > And the old interface is not exported and *only* meant to be used in > compat symbols, different than the current way of handling y2038 > prof code (where the user might still define the ABI with _TIME_SIZE). > > That's why I think alpha compat code should be compartmentalized on > alpha sysdep folder. Agreed. That is the goal with this patch. This patch moves and renames the Alpha functions to make sure there is no conflict with any of the new work. Alistair > > > > >> If Alpha didn't have this requirement we could then just remove the > >> Alpha specific overrides after this series and use all of the new > >> generic y2038 infrastructure. > > > > Also, even if we really are stuck with an Alpha-specific struct > > timeval32, I don't see why that affects the definitions of the > > conversion functions? > > > > zw > >
On Mon, 10 Feb 2020 09:43:20 -0800 Alistair Francis <alistair.francis@wdc.com> wrote: > These functions are alpha specifc, rename them to be clear. > > Let's also rename the header file from tv32-compat.h to > alpha-tv32-compat.h. This is to avoid conflicts with the one we will > introduce later. Reviewed-by: Lukasz Majewski <lukma@denx.de> Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Mon, Feb 10, 2020 at 4:53 PM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 10/02/2020 18:35, Zack Weinberg wrote: > > On Mon, Feb 10, 2020 at 4:25 PM Alistair Francis <alistair23@gmail.com> wrote: > >> On Mon, Feb 10, 2020 at 1:11 PM Zack Weinberg <zackw@panix.com> wrote: > >>> > >>> On Mon, Feb 10, 2020 at 12:50 PM Alistair Francis > >>> <alistair.francis@wdc.com> wrote: > >>>> > >>>> These functions are alpha specifc, rename them to be clear. > >>> > >>> These functions were meant to be generic. I put the file in > >>> s/u/s/l/alpha because it was only *used* on Alpha, at the time. > >>> > >>> What's stopping you from using these functions on all architectures? > >> > >> The Alpha functions use 32-bit versions of the timeval struct for a > >> 64-bit architecture. For all other architectures we want to use the > >> word size version (regardless of time_t size) for the timeval struct. > >> That's the main difference. > > > > I'm sorry, I haven't been following the discussion all that closely. > > Why is it necessary to use a 32-bit timeval struct (with 32-bit > > time_t, I presume) *only* on Alpha? Is there no way we can smooth > > over this difference so that, as you say, > > Alpha is an outlier here, it is the only 64-bit architecture that did > the 32 bit to 64 bit transition some time ago. All the generic code > assumes that 64-bit architectures always have 64-bit time_t > (__ASSUME_TIME64_SYSCALLS). > > And the old interface is not exported and *only* meant to be used in > compat symbols, different than the current way of handling y2038 > prof code (where the user might still define the ABI with _TIME_SIZE). > > That's why I think alpha compat code should be compartmentalized on > alpha sysdep folder. This doesn't address any of my concerns. It should not be necessary to duplicate an *internal header* full of functions whose operation is, or ought to be, completely generic, just because the exposed API is different on Alpha. zw
On 10/02/2020 22:10, Zack Weinberg wrote: > On Mon, Feb 10, 2020 at 4:53 PM Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: >> On 10/02/2020 18:35, Zack Weinberg wrote: >>> On Mon, Feb 10, 2020 at 4:25 PM Alistair Francis <alistair23@gmail.com> wrote: >>>> On Mon, Feb 10, 2020 at 1:11 PM Zack Weinberg <zackw@panix.com> wrote: >>>>> >>>>> On Mon, Feb 10, 2020 at 12:50 PM Alistair Francis >>>>> <alistair.francis@wdc.com> wrote: >>>>>> >>>>>> These functions are alpha specifc, rename them to be clear. >>>>> >>>>> These functions were meant to be generic. I put the file in >>>>> s/u/s/l/alpha because it was only *used* on Alpha, at the time. >>>>> >>>>> What's stopping you from using these functions on all architectures? >>>> >>>> The Alpha functions use 32-bit versions of the timeval struct for a >>>> 64-bit architecture. For all other architectures we want to use the >>>> word size version (regardless of time_t size) for the timeval struct. >>>> That's the main difference. >>> >>> I'm sorry, I haven't been following the discussion all that closely. >>> Why is it necessary to use a 32-bit timeval struct (with 32-bit >>> time_t, I presume) *only* on Alpha? Is there no way we can smooth >>> over this difference so that, as you say, >> >> Alpha is an outlier here, it is the only 64-bit architecture that did >> the 32 bit to 64 bit transition some time ago. All the generic code >> assumes that 64-bit architectures always have 64-bit time_t >> (__ASSUME_TIME64_SYSCALLS). >> >> And the old interface is not exported and *only* meant to be used in >> compat symbols, different than the current way of handling y2038 >> prof code (where the user might still define the ABI with _TIME_SIZE). >> >> That's why I think alpha compat code should be compartmentalized on >> alpha sysdep folder. > > This doesn't address any of my concerns. It should not be necessary > to duplicate an *internal header* full of functions whose operation > is, or ought to be, completely generic, just because the exposed API > is different on Alpha. The 32-bit timeval struct are alpha specific in a sense that no other 64-bit architectures have 32 time_t. We could certainly make it generic and add even more internally pre-processor magic to fit alpha code in generic definitions, but I think it is really a worthless complication. It is a legacy API, and it is highly unlikely that any other port will use such code. I tend to see that generic code, such as the syscall consolidation I have done over the years, should be defined when either you have multiples ABIs that use it or if is the default for newer ports (such as generic kABI). Outliers implementations, such osf alpha syscalls, should be secluded in alpha-only code.
On Tue, Feb 11, 2020 at 8:25 AM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 10/02/2020 22:10, Zack Weinberg wrote: > > > > This doesn't address any of my concerns. It should not be necessary > > to duplicate an *internal header* full of functions whose operation > > is, or ought to be, completely generic, just because the exposed API > > is different on Alpha. > > The 32-bit timeval struct are alpha specific in a sense that no other > 64-bit architectures have 32 time_t. > > We could certainly make it generic and add even more internally > pre-processor magic to fit alpha code in generic definitions, but I > think it is really a worthless complication. It is a legacy API, > and it is highly unlikely that any other port will use such code. I think we're talking past each other. This patch is about tv32-compat.h. tv32-compat.h contains conversion functions between e.g. struct timeval with 32-bit time_t and struct timeval with 64-bit time_t. I still don't see any reason why these conversion functions need to be different on Alpha than they are on the 32-bit architectures. Alpha is unusual in that certain legacy code paths need to call these conversion functions, but *what the functions do* should be identical to what they do on 32-bit architectures. There should be no need for preprocessor magic or anything. zw
On Tue, Feb 11, 2020 at 6:39 AM Zack Weinberg <zackw@panix.com> wrote: > > On Tue, Feb 11, 2020 at 8:25 AM Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: > > On 10/02/2020 22:10, Zack Weinberg wrote: > > > > > > This doesn't address any of my concerns. It should not be necessary > > > to duplicate an *internal header* full of functions whose operation > > > is, or ought to be, completely generic, just because the exposed API > > > is different on Alpha. > > > > The 32-bit timeval struct are alpha specific in a sense that no other > > 64-bit architectures have 32 time_t. > > > > We could certainly make it generic and add even more internally > > pre-processor magic to fit alpha code in generic definitions, but I > > think it is really a worthless complication. It is a legacy API, > > and it is highly unlikely that any other port will use such code. > > I think we're talking past each other. This patch is about > tv32-compat.h. tv32-compat.h contains conversion functions between > e.g. struct timeval with 32-bit time_t and struct timeval with 64-bit > time_t. I still don't see any reason why these conversion functions The generic patches added by this series convert between a 64-bit time_t and a wordsize time_t. Alpha is different in that it converts between a 64-bit time_t and a 32-bit time_t on a 64-bit arch. That is why Adhemerval is saying we would need some preprocessor magic to ensure that for Alpha we always convert to/from a 32-bit time_t on a 64-bit arch. > need to be different on Alpha than they are on the 32-bit > architectures. My understanding is that Alpha is a 64-bit architecture (correct me if I'm wrong, that's just from Wikipedia). The generic patches convert time_t to the architectures wordsize before passing it to the kernel (either 32-bit or 64-bit) while Alpha is different here. > > Alpha is unusual in that certain legacy code paths need to call these > conversion functions, but *what the functions do* should be identical > to what they do on 32-bit architectures. There should be no need for > preprocessor magic or anything. Yes, they are the same as the 32-bit architectures, but not the 64-bit architectures. Everything else in this patch is generic for both 32-bit and 64-bit (except for Alpha). Alistair > > zw
On Tue, Feb 11, 2020 at 11:06 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Tue, Feb 11, 2020 at 6:39 AM Zack Weinberg <zackw@panix.com> wrote: > > > > On Tue, Feb 11, 2020 at 8:25 AM Adhemerval Zanella > > <adhemerval.zanella@linaro.org> wrote: > > > On 10/02/2020 22:10, Zack Weinberg wrote: > > > > > > > > This doesn't address any of my concerns. It should not be necessary > > > > to duplicate an *internal header* full of functions whose operation > > > > is, or ought to be, completely generic, just because the exposed API > > > > is different on Alpha. > > > > > > The 32-bit timeval struct are alpha specific in a sense that no other > > > 64-bit architectures have 32 time_t. > > > > > > We could certainly make it generic and add even more internally > > > pre-processor magic to fit alpha code in generic definitions, but I > > > think it is really a worthless complication. It is a legacy API, > > > and it is highly unlikely that any other port will use such code. > > > > I think we're talking past each other. This patch is about > > tv32-compat.h. tv32-compat.h contains conversion functions between > > e.g. struct timeval with 32-bit time_t and struct timeval with 64-bit > > time_t. I still don't see any reason why these conversion functions > > The generic patches added by this series convert between a 64-bit > time_t and a wordsize time_t. Alpha is different in that it converts > between a 64-bit time_t and a 32-bit time_t on a 64-bit arch. Yes, I understand that, but the definition of the 32-bit-time_t struct timeval is struct __timeval32 { __time32_t tv_sec; __suseconds_t tv_usec; }; either way, isn't it? So the conversion functions shouldn't care? (If that's not what the definition of the 32-bit-time_t struct timeval is, then I would like to know why, in *excruciating* detail.) zw
On 11/02/2020 13:23, Zack Weinberg wrote: > On Tue, Feb 11, 2020 at 11:06 AM Alistair Francis <alistair23@gmail.com> wrote: >> >> On Tue, Feb 11, 2020 at 6:39 AM Zack Weinberg <zackw@panix.com> wrote: >>> >>> On Tue, Feb 11, 2020 at 8:25 AM Adhemerval Zanella >>> <adhemerval.zanella@linaro.org> wrote: >>>> On 10/02/2020 22:10, Zack Weinberg wrote: >>>>> >>>>> This doesn't address any of my concerns. It should not be necessary >>>>> to duplicate an *internal header* full of functions whose operation >>>>> is, or ought to be, completely generic, just because the exposed API >>>>> is different on Alpha. >>>> >>>> The 32-bit timeval struct are alpha specific in a sense that no other >>>> 64-bit architectures have 32 time_t. >>>> >>>> We could certainly make it generic and add even more internally >>>> pre-processor magic to fit alpha code in generic definitions, but I >>>> think it is really a worthless complication. It is a legacy API, >>>> and it is highly unlikely that any other port will use such code. >>> >>> I think we're talking past each other. This patch is about >>> tv32-compat.h. tv32-compat.h contains conversion functions between >>> e.g. struct timeval with 32-bit time_t and struct timeval with 64-bit >>> time_t. I still don't see any reason why these conversion functions >> >> The generic patches added by this series convert between a 64-bit >> time_t and a wordsize time_t. Alpha is different in that it converts >> between a 64-bit time_t and a 32-bit time_t on a 64-bit arch. > > Yes, I understand that, but the definition of the 32-bit-time_t struct > timeval is > > struct __timeval32 { > __time32_t tv_sec; > __suseconds_t tv_usec; > }; > > either way, isn't it? So the conversion functions shouldn't care? > > (If that's not what the definition of the 32-bit-time_t struct timeval > is, then I would like to know why, in *excruciating* detail.) Yes, but why expose this specific types on architectures that do not really require it? Sure we could make this code generic, but again this will be used solely by alpha.
On Tue, Feb 11, 2020 at 12:14 PM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 11/02/2020 13:23, Zack Weinberg wrote: > > On Tue, Feb 11, 2020 at 11:06 AM Alistair Francis <alistair23@gmail.com> wrote: > >> > >> On Tue, Feb 11, 2020 at 6:39 AM Zack Weinberg <zackw@panix.com> wrote: > >>> > >>> On Tue, Feb 11, 2020 at 8:25 AM Adhemerval Zanella > >>> <adhemerval.zanella@linaro.org> wrote: > >>>> On 10/02/2020 22:10, Zack Weinberg wrote: > >>>>> > >>>>> This doesn't address any of my concerns. It should not be necessary > >>>>> to duplicate an *internal header* full of functions whose operation > >>>>> is, or ought to be, completely generic, just because the exposed API > >>>>> is different on Alpha. > >>>> > >>>> The 32-bit timeval struct are alpha specific in a sense that no other > >>>> 64-bit architectures have 32 time_t. > >>>> > >>>> We could certainly make it generic and add even more internally > >>>> pre-processor magic to fit alpha code in generic definitions, but I > >>>> think it is really a worthless complication. It is a legacy API, > >>>> and it is highly unlikely that any other port will use such code. > >>> > >>> I think we're talking past each other. This patch is about > >>> tv32-compat.h. tv32-compat.h contains conversion functions between > >>> e.g. struct timeval with 32-bit time_t and struct timeval with 64-bit > >>> time_t. I still don't see any reason why these conversion functions > >> > >> The generic patches added by this series convert between a 64-bit > >> time_t and a wordsize time_t. Alpha is different in that it converts > >> between a 64-bit time_t and a 32-bit time_t on a 64-bit arch. > > > > Yes, I understand that, but the definition of the 32-bit-time_t struct > > timeval is > > > > struct __timeval32 { > > __time32_t tv_sec; > > __suseconds_t tv_usec; > > }; > > > > either way, isn't it? So the conversion functions shouldn't care? > > > > (If that's not what the definition of the 32-bit-time_t struct timeval > > is, then I would like to know why, in *excruciating* detail.) > > Yes, but why expose this specific types on architectures that do not > really require it? Sure we could make this code generic, but again > this will be used solely by alpha. Huh? Isn't it the exact same definition that would be used on the 32-bit architectures? All the variation should be hiding in the definition of __time32_t? zw
On Tue, Feb 11, 2020 at 8:23 AM Zack Weinberg <zackw@panix.com> wrote: > > On Tue, Feb 11, 2020 at 11:06 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > On Tue, Feb 11, 2020 at 6:39 AM Zack Weinberg <zackw@panix.com> wrote: > > > > > > On Tue, Feb 11, 2020 at 8:25 AM Adhemerval Zanella > > > <adhemerval.zanella@linaro.org> wrote: > > > > On 10/02/2020 22:10, Zack Weinberg wrote: > > > > > > > > > > This doesn't address any of my concerns. It should not be necessary > > > > > to duplicate an *internal header* full of functions whose operation > > > > > is, or ought to be, completely generic, just because the exposed API > > > > > is different on Alpha. > > > > > > > > The 32-bit timeval struct are alpha specific in a sense that no other > > > > 64-bit architectures have 32 time_t. > > > > > > > > We could certainly make it generic and add even more internally > > > > pre-processor magic to fit alpha code in generic definitions, but I > > > > think it is really a worthless complication. It is a legacy API, > > > > and it is highly unlikely that any other port will use such code. > > > > > > I think we're talking past each other. This patch is about > > > tv32-compat.h. tv32-compat.h contains conversion functions between > > > e.g. struct timeval with 32-bit time_t and struct timeval with 64-bit > > > time_t. I still don't see any reason why these conversion functions > > > > The generic patches added by this series convert between a 64-bit > > time_t and a wordsize time_t. Alpha is different in that it converts > > between a 64-bit time_t and a 32-bit time_t on a 64-bit arch. > > Yes, I understand that, but the definition of the 32-bit-time_t struct > timeval is > > struct __timeval32 { > __time32_t tv_sec; > __suseconds_t tv_usec; > }; This is what the Alpha specific calls use. The generic one uses (the name is confusing, I thought I had changed it) struct __timeval32 { long tv_sec; /* Seconds. */ long tv_usec; /* Microseconds. */ }; This is because, as I mentioned earlier, that the kernel expects the time to be the wordsize, regardless of the time_t size (except for Alpha). Using long types means that we can do the same thing for 32-bit and 64-bit architectures. My v1 patchset had a specific 32-bit only version (which didn't apply to 64-bit arches) but this didn't work with ARM, see the discussion here: https://patchwork.ozlabs.org/patch/1232973/ Alistair > > either way, isn't it? So the conversion functions shouldn't care? > > (If that's not what the definition of the 32-bit-time_t struct timeval > is, then I would like to know why, in *excruciating* detail.) > > zw
On Tue, Feb 11, 2020 at 9:56 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Tue, Feb 11, 2020 at 8:23 AM Zack Weinberg <zackw@panix.com> wrote: > > > > On Tue, Feb 11, 2020 at 11:06 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > > > On Tue, Feb 11, 2020 at 6:39 AM Zack Weinberg <zackw@panix.com> wrote: > > > > > > > > On Tue, Feb 11, 2020 at 8:25 AM Adhemerval Zanella > > > > <adhemerval.zanella@linaro.org> wrote: > > > > > On 10/02/2020 22:10, Zack Weinberg wrote: > > > > > > > > > > > > This doesn't address any of my concerns. It should not be necessary > > > > > > to duplicate an *internal header* full of functions whose operation > > > > > > is, or ought to be, completely generic, just because the exposed API > > > > > > is different on Alpha. > > > > > > > > > > The 32-bit timeval struct are alpha specific in a sense that no other > > > > > 64-bit architectures have 32 time_t. > > > > > > > > > > We could certainly make it generic and add even more internally > > > > > pre-processor magic to fit alpha code in generic definitions, but I > > > > > think it is really a worthless complication. It is a legacy API, > > > > > and it is highly unlikely that any other port will use such code. > > > > > > > > I think we're talking past each other. This patch is about > > > > tv32-compat.h. tv32-compat.h contains conversion functions between > > > > e.g. struct timeval with 32-bit time_t and struct timeval with 64-bit > > > > time_t. I still don't see any reason why these conversion functions > > > > > > The generic patches added by this series convert between a 64-bit > > > time_t and a wordsize time_t. Alpha is different in that it converts > > > between a 64-bit time_t and a 32-bit time_t on a 64-bit arch. > > > > Yes, I understand that, but the definition of the 32-bit-time_t struct > > timeval is > > > > struct __timeval32 { > > __time32_t tv_sec; > > __suseconds_t tv_usec; > > }; > > This is what the Alpha specific calls use. > > The generic one uses (the name is confusing, I thought I had changed it) > > struct __timeval32 > { > long tv_sec; /* Seconds. */ > long tv_usec; /* Microseconds. */ > }; I have changed the name of this struct to __timeval_long, hopefully that fixes some of the confusion. The name was left over from v1. Alistair > > This is because, as I mentioned earlier, that the kernel expects the > time to be the wordsize, regardless of the time_t size (except for > Alpha). > > Using long types means that we can do the same thing for 32-bit and > 64-bit architectures. > > My v1 patchset had a specific 32-bit only version (which didn't apply > to 64-bit arches) but this didn't work with ARM, see the discussion > here: https://patchwork.ozlabs.org/patch/1232973/ > > Alistair > > > > > either way, isn't it? So the conversion functions shouldn't care? > > > > (If that's not what the definition of the 32-bit-time_t struct timeval > > is, then I would like to know why, in *excruciating* detail.) > > > > zw
On Tue, Feb 11, 2020 at 1:04 PM Alistair Francis <alistair23@gmail.com> wrote: > > Yes, I understand that, but the definition of the 32-bit-time_t struct > > timeval is > > > > struct __timeval32 { > > __time32_t tv_sec; > > __suseconds_t tv_usec; > > }; > > This is what the Alpha specific calls use. > The generic one uses (the name is confusing, I thought I had changed it) > > struct __timeval32 > { > long tv_sec; /* Seconds. */ > long tv_usec; /* Microseconds. */ > }; > > This is because, as I mentioned earlier, that the kernel expects the > time to be the wordsize, regardless of the time_t size (except for > Alpha). I think that's profoundly wrong. I think we should be using a definition along the lines of struct __timeval32 { __time32_t tv_sec; __suseconds32_t tv_usec; }; on all architectures, with all necessary variation handled within the definitions of __time32_t and __suseconds32_t. That way it can all live in bits/typesizes.h which has to be system-specific regardless. Please consider this a sustained objection to the entire patchset, not just this one patch, unless you can come back with a convincing reason why the above is not possible or would actually make life more complicated. (Incidentally, since it is probably going to come up yet again if we proceed down this path, I still believe that the specification of `long tv_nsec` in struct timespec is a defect in C11 and POSIX and we should ignore both standards and use a typedef name for it.) zw
On Tue, Feb 11, 2020 at 10:38 AM Zack Weinberg <zackw@panix.com> wrote: > > On Tue, Feb 11, 2020 at 1:04 PM Alistair Francis <alistair23@gmail.com> wrote: > > > Yes, I understand that, but the definition of the 32-bit-time_t struct > > > timeval is > > > > > > struct __timeval32 { > > > __time32_t tv_sec; > > > __suseconds_t tv_usec; > > > }; > > > > This is what the Alpha specific calls use. > > The generic one uses (the name is confusing, I thought I had changed it) > > > > struct __timeval32 > > { > > long tv_sec; /* Seconds. */ > > long tv_usec; /* Microseconds. */ > > }; > > > > This is because, as I mentioned earlier, that the kernel expects the > > time to be the wordsize, regardless of the time_t size (except for > > Alpha). > > I think that's profoundly wrong. I think we should be using a > definition along the lines of > > struct __timeval32 { > __time32_t tv_sec; > __suseconds32_t tv_usec; > }; > > on all architectures, with all necessary variation handled within the > definitions of __time32_t and __suseconds32_t. That way it can all > live in bits/typesizes.h which has to be system-specific regardless. When you say all architectures, you mean just 32-bit architectures and Alpha right? This is what my v1 did, and the general consensus was against that as it didn't work for all 32-bit architectures (see the discussion here: https://patchwork.ozlabs.org/patch/1232973/). > > Please consider this a sustained objection to the entire patchset, not > just this one patch, unless you can come back with a convincing reason > why the above is not possible or would actually make life more > complicated. What are you proposing I do instead? > > (Incidentally, since it is probably going to come up yet again if we > proceed down this path, I still believe that the specification of > `long tv_nsec` in struct timespec is a defect in C11 and POSIX and we > should ignore both standards and use a typedef name for it.) I'll leave this for others to comment on, I don't really mind either way but ignoring the standard seems like a strange choice. Alistair > > zw
11.02.2020 в 09:56:59 -0800 Alistair Francis написал(а): > On Tue, Feb 11, 2020 at 8:23 AM Zack Weinberg <zackw@panix.com> wrote: > > > > On Tue, Feb 11, 2020 at 11:06 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > > > On Tue, Feb 11, 2020 at 6:39 AM Zack Weinberg <zackw@panix.com> wrote: > > > > > > > > On Tue, Feb 11, 2020 at 8:25 AM Adhemerval Zanella > > > > <adhemerval.zanella@linaro.org> wrote: > > > > > On 10/02/2020 22:10, Zack Weinberg wrote: > > > > > > > > > > > > This doesn't address any of my concerns. It should not be necessary > > > > > > to duplicate an *internal header* full of functions whose operation > > > > > > is, or ought to be, completely generic, just because the exposed API > > > > > > is different on Alpha. > > > > > > > > > > The 32-bit timeval struct are alpha specific in a sense that no other > > > > > 64-bit architectures have 32 time_t. > > > > > > > > > > We could certainly make it generic and add even more internally > > > > > pre-processor magic to fit alpha code in generic definitions, but I > > > > > think it is really a worthless complication. It is a legacy API, > > > > > and it is highly unlikely that any other port will use such code. > > > > > > > > I think we're talking past each other. This patch is about > > > > tv32-compat.h. tv32-compat.h contains conversion functions between > > > > e.g. struct timeval with 32-bit time_t and struct timeval with 64-bit > > > > time_t. I still don't see any reason why these conversion functions > > > > > > The generic patches added by this series convert between a 64-bit > > > time_t and a wordsize time_t. Alpha is different in that it converts > > > between a 64-bit time_t and a 32-bit time_t on a 64-bit arch. > > > > Yes, I understand that, but the definition of the 32-bit-time_t struct > > timeval is > > > > struct __timeval32 { > > __time32_t tv_sec; > > __suseconds_t tv_usec; > > }; > > This is what the Alpha specific calls use. > > The generic one uses (the name is confusing, I thought I had changed it) > > struct __timeval32 > { > long tv_sec; /* Seconds. */ > long tv_usec; /* Microseconds. */ > }; This probably won't work for x32 and sparc64. I suspect that it will be much simpler to have __timeval32 with 32-bit fields and then do something like this: int __getitimer64 (__itimer_which_t which, struct __itimerval64 *curr_value) { #if KERNEL_OLD_TIMEVAL_IS_TIMEVAL64 return INLINE_SYSCALL_CALL (getitimer, which, &curr_value); #else struct __itimerval32 curr_value_32; if (INLINE_SYSCALL_CALL (getitimer, which, &curr_value_32) == -1) return -1; /* Write all fields of 'curr_value' regardless of overflow. */ curr_value->it_interval = valid_timeval32_to_timeval64 (curr_value_32.it_interval); curr_value->it_value = valid_timeval32_to_timeval64 (curr_value_32.it_value); return 0; #endif } where KERNEL_OLD_TIMEVAL_IS_TIMEVAL64 needs to be true on 64-bit architectures and x32.
On Tue, Feb 11, 2020 at 12:15 PM Stepan Golosunov <stepan@golosunov.pp.ru> wrote: > > 11.02.2020 в 09:56:59 -0800 Alistair Francis написал(а): > > On Tue, Feb 11, 2020 at 8:23 AM Zack Weinberg <zackw@panix.com> wrote: > > > > > > On Tue, Feb 11, 2020 at 11:06 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > > > > > On Tue, Feb 11, 2020 at 6:39 AM Zack Weinberg <zackw@panix.com> wrote: > > > > > > > > > > On Tue, Feb 11, 2020 at 8:25 AM Adhemerval Zanella > > > > > <adhemerval.zanella@linaro.org> wrote: > > > > > > On 10/02/2020 22:10, Zack Weinberg wrote: > > > > > > > > > > > > > > This doesn't address any of my concerns. It should not be necessary > > > > > > > to duplicate an *internal header* full of functions whose operation > > > > > > > is, or ought to be, completely generic, just because the exposed API > > > > > > > is different on Alpha. > > > > > > > > > > > > The 32-bit timeval struct are alpha specific in a sense that no other > > > > > > 64-bit architectures have 32 time_t. > > > > > > > > > > > > We could certainly make it generic and add even more internally > > > > > > pre-processor magic to fit alpha code in generic definitions, but I > > > > > > think it is really a worthless complication. It is a legacy API, > > > > > > and it is highly unlikely that any other port will use such code. > > > > > > > > > > I think we're talking past each other. This patch is about > > > > > tv32-compat.h. tv32-compat.h contains conversion functions between > > > > > e.g. struct timeval with 32-bit time_t and struct timeval with 64-bit > > > > > time_t. I still don't see any reason why these conversion functions > > > > > > > > The generic patches added by this series convert between a 64-bit > > > > time_t and a wordsize time_t. Alpha is different in that it converts > > > > between a 64-bit time_t and a 32-bit time_t on a 64-bit arch. > > > > > > Yes, I understand that, but the definition of the 32-bit-time_t struct > > > timeval is > > > > > > struct __timeval32 { > > > __time32_t tv_sec; > > > __suseconds_t tv_usec; > > > }; > > > > This is what the Alpha specific calls use. > > > > The generic one uses (the name is confusing, I thought I had changed it) > > > > struct __timeval32 > > { > > long tv_sec; /* Seconds. */ > > long tv_usec; /* Microseconds. */ > > }; > > This probably won't work for x32 and sparc64. > > I suspect that it will be much simpler to have __timeval32 with 32-bit > fields and then do something like this: > > int > __getitimer64 (__itimer_which_t which, struct __itimerval64 *curr_value) > { > #if KERNEL_OLD_TIMEVAL_IS_TIMEVAL64 > return INLINE_SYSCALL_CALL (getitimer, which, &curr_value); > #else > struct __itimerval32 curr_value_32; > if (INLINE_SYSCALL_CALL (getitimer, which, &curr_value_32) == -1) > return -1; > > /* Write all fields of 'curr_value' regardless of overflow. */ > curr_value->it_interval > = valid_timeval32_to_timeval64 (curr_value_32.it_interval); > curr_value->it_value > = valid_timeval32_to_timeval64 (curr_value_32.it_value); > return 0; > #endif > } > > where KERNEL_OLD_TIMEVAL_IS_TIMEVAL64 needs to be true on 64-bit > architectures and x32. This works for me. The files apply to all architectures (so we don't have the issues where it doesn't take effect for ARM). It fixes the RV32 issues and it means I can cleanup the Alpha code. I have made these changes and am testing now. Alistair
diff --git a/sysdeps/unix/sysv/linux/alpha/tv32-compat.h b/sysdeps/unix/sysv/linux/alpha/alpha-tv32-compat.h similarity index 88% rename from sysdeps/unix/sysv/linux/alpha/tv32-compat.h rename to sysdeps/unix/sysv/linux/alpha/alpha-tv32-compat.h index 8e34ed1c1b..3073005c65 100644 --- a/sysdeps/unix/sysv/linux/alpha/tv32-compat.h +++ b/sysdeps/unix/sysv/linux/alpha/alpha-tv32-compat.h @@ -70,13 +70,13 @@ struct rusage32 overflow, they write { INT32_MAX, TV_USEC_MAX } to the output. */ static inline struct timeval -valid_timeval32_to_timeval (const struct timeval32 tv) +alpha_valid_timeval32_to_timeval (const struct timeval32 tv) { return (struct timeval) { tv.tv_sec, tv.tv_usec }; } static inline struct timeval32 -valid_timeval_to_timeval32 (const struct timeval tv64) +alpha_valid_timeval_to_timeval32 (const struct timeval tv64) { if (__glibc_unlikely (tv64.tv_sec > (time_t) INT32_MAX)) return (struct timeval32) { INT32_MAX, TV_USEC_MAX}; @@ -84,27 +84,27 @@ valid_timeval_to_timeval32 (const struct timeval tv64) } static inline struct timespec -valid_timeval32_to_timespec (const struct timeval32 tv) +alpha_valid_timeval32_to_timespec (const struct timeval32 tv) { return (struct timespec) { tv.tv_sec, tv.tv_usec * 1000 }; } static inline struct timeval32 -valid_timespec_to_timeval32 (const struct timespec ts) +alpha_valid_timespec_to_timeval32 (const struct timespec ts) { return (struct timeval32) { (time_t) ts.tv_sec, ts.tv_nsec / 1000 }; } static inline void -rusage64_to_rusage32 (struct rusage32 *restrict r32, +alpha_rusage64_to_rusage32 (struct rusage32 *restrict r32, const struct rusage *restrict r64) { /* Make sure the entire output structure is cleared, including padding and reserved fields. */ memset (r32, 0, sizeof *r32); - r32->ru_utime = valid_timeval_to_timeval32 (r64->ru_utime); - r32->ru_stime = valid_timeval_to_timeval32 (r64->ru_stime); + r32->ru_utime = alpha_valid_timeval_to_timeval32 (r64->ru_utime); + r32->ru_stime = alpha_valid_timeval_to_timeval32 (r64->ru_stime); r32->ru_maxrss = r64->ru_maxrss; r32->ru_ixrss = r64->ru_ixrss; r32->ru_idrss = r64->ru_idrss; @@ -121,4 +121,4 @@ rusage64_to_rusage32 (struct rusage32 *restrict r32, r32->ru_nivcsw = r64->ru_nivcsw; } -#endif /* tv32-compat.h */ +#endif /* alpha-tv32-compat.h */ diff --git a/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c b/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c index 9825a4734d..f0a1123639 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_adjtime.c @@ -22,7 +22,7 @@ #include <sys/time.h> #include <sys/timex.h> -#include <tv32-compat.h> +#include <alpha-tv32-compat.h> struct timex32 { unsigned int modes; /* mode selector */ @@ -57,13 +57,13 @@ int attribute_compat_text_section __adjtime_tv32 (const struct timeval32 *itv, struct timeval32 *otv) { - struct timeval itv64 = valid_timeval32_to_timeval (*itv); + struct timeval itv64 = alpha_valid_timeval32_to_timeval (*itv); struct timeval otv64; if (__adjtime (&itv64, &otv64) == -1) return -1; - *otv = valid_timeval_to_timeval32 (otv64); + *otv = alpha_valid_timeval_to_timeval32 (otv64); return 0; } @@ -91,7 +91,7 @@ __adjtimex_tv32 (struct timex32 *tx) tx64.calcnt = tx->calcnt; tx64.errcnt = tx->errcnt; tx64.stbcnt = tx->stbcnt; - tx64.time = valid_timeval32_to_timeval (tx->time); + tx64.time = alpha_valid_timeval32_to_timeval (tx->time); int status = __adjtimex (&tx64); if (status < 0) @@ -116,7 +116,7 @@ __adjtimex_tv32 (struct timex32 *tx) tx->calcnt = tx64.calcnt; tx->errcnt = tx64.errcnt; tx->stbcnt = tx64.stbcnt; - tx->time = valid_timeval_to_timeval32 (tx64.time); + tx->time = alpha_valid_timeval_to_timeval32 (tx64.time); return status; } diff --git a/sysdeps/unix/sysv/linux/alpha/osf_getitimer.c b/sysdeps/unix/sysv/linux/alpha/osf_getitimer.c index e9de2b287b..204d4ba796 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_getitimer.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_getitimer.c @@ -21,7 +21,7 @@ #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) #include <sys/time.h> -#include <tv32-compat.h> +#include <alpha-tv32-compat.h> int attribute_compat_text_section @@ -33,9 +33,9 @@ __getitimer_tv32 (int which, struct itimerval32 *curr_value) /* Write all fields of 'curr_value' regardless of overflow. */ curr_value->it_interval - = valid_timeval_to_timeval32 (curr_value_64.it_interval); + = alpha_valid_timeval_to_timeval32 (curr_value_64.it_interval); curr_value->it_value - = valid_timeval_to_timeval32 (curr_value_64.it_value); + = alpha_valid_timeval_to_timeval32 (curr_value_64.it_value); return 0; } diff --git a/sysdeps/unix/sysv/linux/alpha/osf_getrusage.c b/sysdeps/unix/sysv/linux/alpha/osf_getrusage.c index 74c6fb49aa..be81994654 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_getrusage.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_getrusage.c @@ -22,7 +22,7 @@ #include <sys/time.h> #include <sys/resource.h> -#include <tv32-compat.h> +#include <alpha-tv32-compat.h> int __getrusage_tv32 (int who, struct rusage32 *usage32) @@ -31,7 +31,7 @@ __getrusage_tv32 (int who, struct rusage32 *usage32) if (__getrusage (who, &usage64) == -1) return -1; - rusage64_to_rusage32 (usage32, &usage64); + alpha_rusage64_to_rusage32 (usage32, &usage64); return 0; } diff --git a/sysdeps/unix/sysv/linux/alpha/osf_gettimeofday.c b/sysdeps/unix/sysv/linux/alpha/osf_gettimeofday.c index df7f06765b..9ffda2fde3 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_gettimeofday.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_gettimeofday.c @@ -23,7 +23,7 @@ #include <string.h> #include <time.h> #include <sys/time.h> -#include <tv32-compat.h> +#include <alpha-tv32-compat.h> /* Get the current time of day and timezone information putting it into *TV and *TZ. */ @@ -38,7 +38,7 @@ __gettimeofday_tv32 (struct timeval32 *restrict tv32, void *restrict tz) struct timespec ts; __clock_gettime (CLOCK_REALTIME, &ts); - *tv32 = valid_timespec_to_timeval32 (ts); + *tv32 = alpha_valid_timespec_to_timeval32 (ts); return 0; } diff --git a/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c b/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c index 7df2d1b71c..726dfc8b0e 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_setitimer.c @@ -21,7 +21,7 @@ #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) #include <sys/time.h> -#include <tv32-compat.h> +#include <alpha-tv32-compat.h> int attribute_compat_text_section @@ -30,9 +30,9 @@ __setitimer_tv32 (int which, const struct itimerval32 *restrict new_value, { struct itimerval new_value_64; new_value_64.it_interval - = valid_timeval32_to_timeval (new_value->it_interval); + = alpha_valid_timeval32_to_timeval (new_value->it_interval); new_value_64.it_value - = valid_timeval32_to_timeval (new_value->it_value); + = alpha_valid_timeval32_to_timeval (new_value->it_value); if (old_value == NULL) return __setitimer (which, &new_value_64, NULL); @@ -43,9 +43,9 @@ __setitimer_tv32 (int which, const struct itimerval32 *restrict new_value, /* Write all fields of 'old_value' regardless of overflow. */ old_value->it_interval - = valid_timeval_to_timeval32 (old_value_64.it_interval); + = alpha_valid_timeval_to_timeval32 (old_value_64.it_interval); old_value->it_value - = valid_timeval_to_timeval32 (old_value_64.it_value); + = alpha_valid_timeval_to_timeval32 (old_value_64.it_value); return 0; } diff --git a/sysdeps/unix/sysv/linux/alpha/osf_settimeofday.c b/sysdeps/unix/sysv/linux/alpha/osf_settimeofday.c index 6e17a95a47..044363e079 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_settimeofday.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_settimeofday.c @@ -23,7 +23,7 @@ #include <sys/time.h> #include <time.h> #include <errno.h> -#include <tv32-compat.h> +#include <alpha-tv32-compat.h> /* Set the current time of day and timezone information. This call is restricted to the super-user. */ @@ -42,7 +42,7 @@ __settimeofday_tv32 (const struct timeval32 *tv32, return __settimezone (tz); } - struct timespec ts = valid_timeval32_to_timespec (*tv32); + struct timespec ts = alpha_valid_timeval32_to_timespec (*tv32); return __clock_settime (CLOCK_REALTIME, &ts); } diff --git a/sysdeps/unix/sysv/linux/alpha/osf_utimes.c b/sysdeps/unix/sysv/linux/alpha/osf_utimes.c index 6c3fad0132..8ad9fb567c 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_utimes.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_utimes.c @@ -21,15 +21,15 @@ #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) #include <sys/time.h> -#include <tv32-compat.h> +#include <alpha-tv32-compat.h> int attribute_compat_text_section __utimes_tv32 (const char *filename, const struct timeval32 times32[2]) { struct timeval times[2]; - times[0] = valid_timeval32_to_timeval (times32[0]); - times[1] = valid_timeval32_to_timeval (times32[1]); + times[0] = alpha_valid_timeval32_to_timeval (times32[0]); + times[1] = alpha_valid_timeval32_to_timeval (times32[1]); return __utimes (filename, times); } diff --git a/sysdeps/unix/sysv/linux/alpha/osf_wait4.c b/sysdeps/unix/sysv/linux/alpha/osf_wait4.c index 6af8347871..c664e8e93f 100644 --- a/sysdeps/unix/sysv/linux/alpha/osf_wait4.c +++ b/sysdeps/unix/sysv/linux/alpha/osf_wait4.c @@ -23,7 +23,7 @@ #include <sys/time.h> #include <sys/resource.h> #include <sys/wait.h> -#include <tv32-compat.h> +#include <alpha-tv32-compat.h> pid_t attribute_compat_text_section @@ -33,7 +33,7 @@ __wait4_tv32 (pid_t pid, int *status, int options, struct rusage32 *usage32) pid_t child = __wait4 (pid, status, options, &usage64); if (child >= 0 && usage32 != NULL) - rusage64_to_rusage32 (usage32, &usage64); + alpha_rusage64_to_rusage32 (usage32, &usage64); return child; }