diff mbox series

[v3,02/29] Prepare for the addition of the <sys/pagesize.h> header

Message ID 2f833cc18d427bf1db6c2653f9ff43a69a5baa70.1727624528.git.fweimer@redhat.com
State New
Headers show
Series Teach glibc about possible page sizes and handle gaps in ld.so | expand

Commit Message

Florian Weimer Sept. 29, 2024, 3:49 p.m. UTC
Add a stub version of <bits/pagesize.h>.  This header is similar to
<bits/wordsize.h> in that every port must define its own version.

Add <sys/pagesize.h>, which will become the installed version
of this header.
---
 bits/pagesize.h        | 10 ++++++++++
 include/sys/pagesize.h |  1 +
 misc/sys/pagesize.h    | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)
 create mode 100644 bits/pagesize.h
 create mode 100644 include/sys/pagesize.h
 create mode 100644 misc/sys/pagesize.h

Comments

Paul Eggert Sept. 29, 2024, 7:06 p.m. UTC | #1
On 2024-09-29 08:49, Florian Weimer wrote:
> +#define PAGE_SIZE_MIN (1UL << __GLIBC_PAGE_SHIFT_MIN)
> +#define PAGE_SIZE_MAX (1UL << __GLIBC_PAGE_SHIFT_MAX)

Shouldn't these have the same type as sysconf (_SC_PAGESIZE)? It seems 
odd to introduce yet another type for page sizes.

(I write this as a longtime fan of signed types due to the better 
checking for them, so yeah, I'm biased....)
Florian Weimer Sept. 29, 2024, 7:47 p.m. UTC | #2
* Paul Eggert:

> On 2024-09-29 08:49, Florian Weimer wrote:
>> +#define PAGE_SIZE_MIN (1UL << __GLIBC_PAGE_SHIFT_MIN)
>> +#define PAGE_SIZE_MAX (1UL << __GLIBC_PAGE_SHIFT_MAX)
>
> Shouldn't these have the same type as sysconf (_SC_PAGESIZE)? It seems
> odd to introduce yet another type for page sizes.

Well, getpagesize returns an int, so there's not much consistency to be
had here.

I can turn it into 1L if you prefer that, but byte counts are generally
measured in size_t …

Thanks,
Florian
Paul Eggert Sept. 29, 2024, 8:02 p.m. UTC | #3
On 2024-09-29 12:47, Florian Weimer wrote:
> * Paul Eggert:
> 
>> On 2024-09-29 08:49, Florian Weimer wrote:
>>> +#define PAGE_SIZE_MIN (1UL << __GLIBC_PAGE_SHIFT_MIN)
>>> +#define PAGE_SIZE_MAX (1UL << __GLIBC_PAGE_SHIFT_MAX)
>>
>> Shouldn't these have the same type as sysconf (_SC_PAGESIZE)? It seems
>> odd to introduce yet another type for page sizes.
> 
> Well, getpagesize returns an int, so there's not much consistency to be
> had here.

That's why POSIX removed getpagesize way back in 2001. The function 
shouldn't be used in portable code and is not a good model for page sizes.


> I can turn it into 1L if you prefer that, but byte counts are generally
> measured in size_t …

I do prefer it, partly because of sysconf (_SC_PAGE_SIZE), partly 
because of the general trend in GNU utilities to move away from size_t 
for sizes (admittedly this is a long road to travel).
Florian Weimer Oct. 1, 2024, 4:29 p.m. UTC | #4
* Paul Eggert:

> On 2024-09-29 12:47, Florian Weimer wrote:
>> * Paul Eggert:
>> 
>>> On 2024-09-29 08:49, Florian Weimer wrote:
>>>> +#define PAGE_SIZE_MIN (1UL << __GLIBC_PAGE_SHIFT_MIN)
>>>> +#define PAGE_SIZE_MAX (1UL << __GLIBC_PAGE_SHIFT_MAX)
>>>
>>> Shouldn't these have the same type as sysconf (_SC_PAGESIZE)? It seems
>>> odd to introduce yet another type for page sizes.
>> Well, getpagesize returns an int, so there's not much consistency to
>> be
>> had here.
>
> That's why POSIX removed getpagesize way back in 2001. The function
> shouldn't be used in portable code and is not a good model for page
> sizes.

It's a pitty because getpagesize is easier to optimize than sysconf
(_SC_PAGESIZE).  But we can make both work.  (The series only has the
constant page size optimization, it does not turn sysconf (_SC_PAGESIZE)
into __getpagesize () or local variable access.)

>> I can turn it into 1L if you prefer that, but byte counts are generally
>> measured in size_t …
>
> I do prefer it, partly because of sysconf (_SC_PAGE_SIZE), partly
> because of the general trend in GNU utilities to move away from size_t
> for sizes (admittedly this is a long road to travel).

I don't agree with this rationale, but I really want

  roundup (offset, PAGE_SIZE_MAX)

to do the right thing if offset is off64_t, and that requires using
unsigned long long int on 32-bit—or a signed type.  So the latter it is.
I'll post a v2.

Thanks,
Florian
Maciej W. Rozycki Oct. 1, 2024, 4:56 p.m. UTC | #5
On Tue, 1 Oct 2024, Florian Weimer wrote:

> > That's why POSIX removed getpagesize way back in 2001. The function
> > shouldn't be used in portable code and is not a good model for page
> > sizes.
> 
> It's a pitty because getpagesize is easier to optimize than sysconf
> (_SC_PAGESIZE).  But we can make both work.  (The series only has the
> constant page size optimization, it does not turn sysconf (_SC_PAGESIZE)
> into __getpagesize () or local variable access.)

 Why do you think `getpagesize' is easier to optimise than `sysconf 
(_SC_PAGESIZE)'?

 I can certainly envisage `sysconf' being a static inline function in 
<unistd.h> that handles all the known (or important enough) compile-time 
constants itself where the identifier of the parameter requested is also a 
constant, while deferring to a library call for everything else.

  Maciej
Paul Eggert Oct. 1, 2024, 5:57 p.m. UTC | #6
On 2024-10-01 09:56, Maciej W. Rozycki wrote:
>   I can certainly envisage `sysconf' being a static inline function in
> <unistd.h>

It couldn't be static, as then people couldn't call it from an extern 
inline function (as per ISO C24 §6.7.5 ¶2).

It could be a macro.
Maciej W. Rozycki Oct. 1, 2024, 6:22 p.m. UTC | #7
On Tue, 1 Oct 2024, Paul Eggert wrote:

> >   I can certainly envisage `sysconf' being a static inline function in
> > <unistd.h>
> 
> It couldn't be static, as then people couldn't call it from an extern inline
> function (as per ISO C24 §6.7.5 ¶2).

 And why is it going to be a problem?  An extern inline function is either 
inlined (in which case so are any calls to static inline functions within) 
or the call site defers to an external definition (in which case the body 
is discarded, so it doesn't matter what it calls).  The ISO C24 clause you 
refer says:

"Function specifiers shall be used only in the declaration of an 
identifier for a function."

which doesn't seem relevant to me.  Clearly I'm missing something, so 
please elaborate.

> It could be a macro.

 Ouch!

  Maciej
Paul Eggert Oct. 1, 2024, 6:38 p.m. UTC | #8
On 2024-10-01 11:22, Maciej W. Rozycki wrote:
>   And why is it going to be a problem?

It's in a standard as a constraint, which means standard compilers are 
required to issue a diagnostic, unfortunately.


> The ISO C24 clause you 
> refer says:

Oops, I meant to cite paragraph 3 not paragraph 2. Sorry about that. 
Paragraph 3 says, "An inline definition of a function with external 
linkage shall not ... contain, anywhere in the tokens making up the 
function definition, a reference to an identifier with internal 
linkage." This means a user-defined extern inline function cannot call a 
static function.

I suppose we could do something like this:

   #if __GNUC_PREREQ (3,2) || __glibc_has_attribute (__always_inline__)
   static __always_inline long int
   __glibc_sysconf (int __name)
   {
     ... optimizations go here ...
   }
   # define sysconf(name) __glibc_sysconf (name)
   #endif

This would support the C standard rules, so long as GCC is smart enough 
to not issue a diagnostic about this sort of thing (is it?).
Florian Weimer Oct. 1, 2024, 10:54 p.m. UTC | #9
* Paul Eggert:

> On 2024-10-01 11:22, Maciej W. Rozycki wrote:
>>   And why is it going to be a problem?
>
> It's in a standard as a constraint, which means standard compilers are
> required to issue a diagnostic, unfortunately.

We already have __extern_inline in <sys/cdefs.h> for that.  It's used
for things like bsearch.  This patch adds more inline functions like
that:

  [PATCH v3 29/29] Optimize various ways to obtain the page size using
  <bits/pagesize.h>
  <https://inbox.sourceware.org/libc-alpha/52aa57f9ba1971262b8aaa5c705b2a260e6c09eb.1727624528.git.fweimer@redhat.com/>

The tricky part is that we want to tell GCC that sysconf (_SC_PAGE_SIZE)
is a constant, but sysconf in general is not __attribute__
((__const__)).  The easiest way to implement that is to call
__getpagesize, which has the right attribute.  The patch doesn't
implement that, though.

Thanks,
Florian
Paul Eggert Oct. 2, 2024, 3:46 a.m. UTC | #10
On 2024-10-01 15:54, Florian Weimer wrote:

> We already have __extern_inline in <sys/cdefs.h> for that.

Thanks, didn't know that.


> The tricky part is that we want to tell GCC that sysconf (_SC_PAGE_SIZE)
> is a constant, but sysconf in general is not __attribute__
> ((__const__)).  The easiest way to implement that is to call
> __getpagesize, which has the right attribute.  The patch doesn't
> implement that, though.

Can we do something like the following to make sysconf (_SC_PAGESIZE) 
effectively const?

   #ifdef __extern_always_inline
   __extern_always_inline __attribute_const__ long int
   __glibc_sysconf_const (int __name)
   {
     return sysconf (__name);
   }

   # define sysconf(name) \
      ((__builtin_constant_p (name) \
        && ((name) == _SC_PAGESIZE || (name) == _SC_RE_DUP_MAX)) \
       ? __glibc_sysconf_const (name) \
       : sysconf (name))
   #endif

Of course add any other relevant _SC_... values to the disjunct.
Florian Weimer Oct. 2, 2024, 8:23 a.m. UTC | #11
* Paul Eggert:

> On 2024-10-01 15:54, Florian Weimer wrote:
>> The tricky part is that we want to tell GCC that sysconf (_SC_PAGE_SIZE)
>> is a constant, but sysconf in general is not __attribute__
>> ((__const__)).  The easiest way to implement that is to call
>> __getpagesize, which has the right attribute.  The patch doesn't
>> implement that, though.
>
> Can we do something like the following to make sysconf (_SC_PAGESIZE)
> effectively const?
>
>   #ifdef __extern_always_inline
>   __extern_always_inline __attribute_const__ long int
>   __glibc_sysconf_const (int __name)
>   {
>     return sysconf (__name);
>   }
>
>   # define sysconf(name) \
>      ((__builtin_constant_p (name) \
>        && ((name) == _SC_PAGESIZE || (name) == _SC_RE_DUP_MAX)) \
>       ? __glibc_sysconf_const (name) \
>       : sysconf (name))
>   #endif
>
> Of course add any other relevant _SC_... values to the disjunct.

I think we should avoid the macro, which is possible with __REDIRECT_NTH
and a local const alias, I believe.  We just need to hope that the
compiler applies the const attribute to the alias, and not the called
function.  And we'd probably want to call __getpagesize anyway because
it already exists, so that we don't have to go through that switch
statement in the sysconf implementation.

Thanks,
Florian
Maciej W. Rozycki Oct. 2, 2024, 9:47 a.m. UTC | #12
On Tue, 1 Oct 2024, Paul Eggert wrote:

> >   And why is it going to be a problem?
> 
> It's in a standard as a constraint, which means standard compilers are
> required to issue a diagnostic, unfortunately.

 Thank you, Captain Obvious.

> > The ISO C24 clause you refer says:
> 
> Oops, I meant to cite paragraph 3 not paragraph 2. Sorry about that. Paragraph
> 3 says, "An inline definition of a function with external linkage shall not
> ... contain, anywhere in the tokens making up the function definition, a
> reference to an identifier with internal linkage." This means a user-defined
> extern inline function cannot call a static function.

 Is it a correct interpretation though that this applies to static inline 
function definitions?  Those definitions have always been a type-checked 
equivalent to macros and to make them cause a compilation warning as from 
ISO C24 seems counter-productive (with "always" as in "for 20+ years").

 Needless to say GCC doesn't issue any diagnostics for this program:

static inline int
bar (void)
{
  return 0;
}

extern inline int
foo (void)
{
  return bar ();
}

int
main (void)
{
  return foo ();
}

-- not even at `-Wall -W -pedantic -std=iso9899:2024' (and regardless of 
optimisation or the lack of requested).

> I suppose we could do something like this:
> 
>   #if __GNUC_PREREQ (3,2) || __glibc_has_attribute (__always_inline__)
>   static __always_inline long int
>   __glibc_sysconf (int __name)
>   {
>     ... optimizations go here ...
>   }
>   # define sysconf(name) __glibc_sysconf (name)
>   #endif
> 
> This would support the C standard rules, so long as GCC is smart enough to not
> issue a diagnostic about this sort of thing (is it?).

 This is a definition of a static inline function, the calling of which 
you just told me is not supported from extern inline functions as from ISO 
C24, so I'm even more confused now.

  Maciej
Paul Eggert Oct. 3, 2024, 11:14 p.m. UTC | #13
On 2024-10-02 02:47, Maciej W. Rozycki wrote:

>   Is it a correct interpretation though that this applies to static inline
> function definitions?

I see no other way to interpret it. I agree it's a dumb rule. I don't 
know why the rule is there.


>> I suppose we could do something like this:
>>
>>    #if __GNUC_PREREQ (3,2) || __glibc_has_attribute (__always_inline__)
>>    static __always_inline long int
>>    __glibc_sysconf (int __name)
>>    {
>>      ... optimizations go here ...
>>    }
>>    # define sysconf(name) __glibc_sysconf (name)
>>    #endif
>>
>> This would support the C standard rules, so long as GCC is smart enough to not
>> issue a diagnostic about this sort of thing (is it?).
> 
>   This is a definition of a static inline function, the calling of which
> you just told me is not supported from extern inline functions as from ISO
> C24, so I'm even more confused now.

This function is defined using syntax that is an extension to C, and so 
it does not need to follow all the rules that the standard requires.

The problem I originally referred to was in a user-defined extern inline 
function that calls a system-header-defined static function that does 
not use C extensions. If that's done with GCC I suppose we can talk the 
GCC maintainers into not diagnosing it (which apparently it's already 
doing); but if it's done with some other compiler things would quite 
possibly not be so smooth.
diff mbox series

Patch

diff --git a/bits/pagesize.h b/bits/pagesize.h
new file mode 100644
index 0000000000..d83561d0c1
--- /dev/null
+++ b/bits/pagesize.h
@@ -0,0 +1,10 @@ 
+#error "This file must be written based on the possible page sizes"
+
+/* 1 << __GLIBC_PAGE_SHIFT_MIN is the minimum possible page size.
+   The least significant __GLIBC_PAGE_SHIFT_MIN bits of pointers
+   return by mmap are guaranteed to be cleared.  */
+#define __GLIBC_PAGE_SHIFT_MIN
+
+/* 1 << __GLIBC_PAGE_SHIFT_MAX is the maximum possible page size.
+   On-disk file formats must not require smaller mapping offsets.  */
+#define __GLIBC_PAGE_SHIFT_MAX
diff --git a/include/sys/pagesize.h b/include/sys/pagesize.h
new file mode 100644
index 0000000000..2533ee7e90
--- /dev/null
+++ b/include/sys/pagesize.h
@@ -0,0 +1 @@ 
+#include <misc/sys/pagesize.h>
diff --git a/misc/sys/pagesize.h b/misc/sys/pagesize.h
new file mode 100644
index 0000000000..b54b6e6396
--- /dev/null
+++ b/misc/sys/pagesize.h
@@ -0,0 +1,32 @@ 
+/* Information about supported page sizes.
+   Copyright (C) 2024 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _SYS_PAGESIZE_H
+#define _SYS_PAGESIZE_H
+
+#include <bits/pagesize.h>
+
+/* Minimum and maximum possible page sizes, in bytes.  */
+#define PAGE_SIZE_MIN (1UL << __GLIBC_PAGE_SHIFT_MIN)
+#define PAGE_SIZE_MAX (1UL << __GLIBC_PAGE_SHIFT_MAX)
+
+/* Base-2 logarithm of the size limits.  */
+#define PAGE_SHIFT_MIN __GLIBC_PAGE_SHIFT_MIN
+#define PAGE_SHIFT_MAX __GLIBC_PAGE_SHIFT_MAX
+
+#endif /* _SYS_PAGESIZE_H */