[PATCH v4 0/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[PATCH v4 0/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

Zied Guermazi (1):
  get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

 gdb/nat/linux-btrace.c | 43 ++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

PAGE_SIZE is not defined in all build configurations, e.g cross compiling
for aarch64 GNU/Linux machine. This patch gets the value at runtime using 
posix call sysconf (_SC_PAGESIZE)

---
 gdb/nat/linux-btrace.c | 43 ++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index b0d6dcd7cf1..d99ec8e01e3 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -445,6 +445,17 @@ diagnose_perf_event_open_fail ()
   error (_("Failed to start recording: %s"), safe_strerror (errno));
 }
 
+/* Returns System page size.  */
+
+static long
+system_page_size ()
+{
+  long page_size = sysconf (_SC_PAGESIZE);
+  if (page_size == -1)
+    perror_with_name (_("Failed to get system page size"));
+  return page_size;
+}
+
 /* Enable branch tracing in BTS format.  */
 
 static struct btrace_target_info *
@@ -486,9 +497,10 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
   if (fd.get () < 0)
     diagnose_perf_event_open_fail ();
 
+  long page_size = system_page_size ();
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = ((size_t) conf->size / PAGE_SIZE
-	   + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1));
+  pages = ((size_t) conf->size / page_size
+	   + ((conf->size % page_size) == 0 ? 0 : 1));
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
@@ -507,17 +519,17 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
       size_t length;
       __u64 data_size;
 
-      data_size = (__u64) pages * PAGE_SIZE;
+      data_size = (__u64) pages * (__u64) page_size;
 
       /* Don't ask for more than we can represent in the configuration.  */
       if ((__u64) UINT_MAX < data_size)
 	continue;
 
       size = (size_t) data_size;
-      length = size + PAGE_SIZE;
+      length = size + (size_t) page_size;
 
       /* Check for overflows.  */
-      if ((__u64) length != data_size + PAGE_SIZE)
+      if ((__u64) length != data_size + (__u64) page_size)
 	continue;
 
       errno = 0;
@@ -532,7 +544,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
 
   struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
     data.get ();
-  data_offset = PAGE_SIZE;
+  data_offset = (__u64) page_size;
 
 #if defined (PERF_ATTR_SIZE_VER5)
   if (offsetof (struct perf_event_mmap_page, data_size) <= header->size)
@@ -630,8 +642,9 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
     diagnose_perf_event_open_fail ();
 
   /* Allocate the configuration page. */
-  scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
-		    fd.get (), 0);
+  long page_size = system_page_size ();
+  scoped_mmap data (nullptr, (size_t) page_size, PROT_READ | PROT_WRITE,
+		     MAP_SHARED, fd.get (), 0);
   if (data.get () == MAP_FAILED)
     error (_("Failed to map trace user page: %s."), safe_strerror (errno));
 
@@ -641,8 +654,8 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   header->aux_offset = header->data_offset + header->data_size;
 
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = ((size_t) conf->size / PAGE_SIZE
-	   + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1));
+  pages = ((size_t) conf->size / page_size
+	   + ((conf->size % page_size) == 0 ? 0 : 1));
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
@@ -661,7 +674,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
       size_t length;
       __u64 data_size;
 
-      data_size = (__u64) pages * PAGE_SIZE;
+      data_size = (__u64) pages * (__u64) page_size;
 
       /* Don't ask for more than we can represent in the configuration.  */
       if ((__u64) UINT_MAX < data_size)
@@ -732,7 +745,8 @@ linux_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
 static enum btrace_error
 linux_disable_bts (struct btrace_tinfo_bts *tinfo)
 {
-  munmap((void *) tinfo->header, tinfo->bts.size + PAGE_SIZE);
+  long page_size = system_page_size ();
+  munmap ((void *) tinfo->header, (size_t) (tinfo->bts.size + page_size));
   close (tinfo->file);
 
   return BTRACE_ERR_NONE;
@@ -743,8 +757,9 @@ linux_disable_bts (struct btrace_tinfo_bts *tinfo)
 static enum btrace_error
 linux_disable_pt (struct btrace_tinfo_pt *tinfo)
 {
-  munmap((void *) tinfo->pt.mem, tinfo->pt.size);
-  munmap((void *) tinfo->header, PAGE_SIZE);
+  long page_size = system_page_size ();
+  munmap ((void *) tinfo->pt.mem, tinfo->pt.size);
+  munmap ((void *) tinfo->header, (size_t) page_size);
   close (tinfo->file);
 
   return BTRACE_ERR_NONE;
-- 
2.25.1

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Andreas Schwab]

On Apr 30 2022, Zied Guermazi wrote:

> PAGE_SIZE is not defined in all build configurations, e.g cross compiling
> for aarch64 GNU/Linux machine.

Why is that file compiled in the first place?  The only configuration
using it is native x86-linux.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

hi Andreas,

this is part of the preparation for offering the execution record and 
replay functionality on ARM processors using CoreSight ETM traces. 
Markus Metzger recommended in his review brining this change in a 
separated patch.

Kind Regards

Zied Guermazi

On 30.04.22 14:29, Andreas Schwab wrote:
> On Apr 30 2022, Zied Guermazi wrote:
>
>> PAGE_SIZE is not defined in all build configurations, e.g cross compiling
>> for aarch64 GNU/Linux machine.
> Why is that file compiled in the first place?  The only configuration
> using it is native x86-linux.
>

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Andreas Schwab]

Why is it relevant whether this is cross compiling?

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

hi Andreas,

the issue was first observed when activating this code while compiling 
on an x86 for an arm target.

/Zied

On 30.04.22 14:52, Andreas Schwab wrote:
> Why is it relevant whether this is cross compiling?
>
-- 

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Andreas Schwab]

How is that different from native compilation?

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

Hi Andreas,

PAGE_SIZE for target system was not defined, in other cases PAGE_SIZE of 
the host system was used etc... :).

There are also some systems where the constant PAGE_SIZE is not defined 
at all. This patch is about using the portable posix API instead.

here are some further readings

https://github.com/intel/libipt/issues/11

/Zied


On 30.04.22 15:03, Andreas Schwab wrote:
> How is that different from native compilation?
>
-- 

*Zied Guermazi*
founder

Trande GmbH
Leuschnerstraße 2
69469 Weinheim/Germany

Mobile: +491722645127
mailto:zied.guermazi@trande.de <mailto:zied.guermazi@trande.de>

*Trande GmbH*
Leuschnerstraße 2, D-69469 Weinheim; Telefon: +491722645127
Sitz der Gesellschaft: Weinheim- Registergericht: AG Mannheim HRB 736209 
- Geschäftsführung: Zied Guermazi

*Confidentiality Note*
This message is intended only for the use of the named recipient(s) and 
may contain confidential and/or privileged information. If you are not 
the intended recipient, please contact the sender and delete the 
message. Any unauthorized use of the information contained in this 
message is prohibited.

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Andreas Schwab]

On Apr 30 2022, Zied Guermazi wrote:

> PAGE_SIZE for target system was not defined, in other cases PAGE_SIZE of
> the host system was used etc... :).

This file is only used in a native configuration.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Pedro Alves]

On 2022-04-30 14:33, Andreas Schwab wrote:
> On Apr 30 2022, Zied Guermazi wrote:
> 
>> PAGE_SIZE for target system was not defined, in other cases PAGE_SIZE of
>> the host system was used etc... :).
> 
> This file is only used in a native configuration.
> 

Zied, when you said:

  "the issue was first observed when activating this code while compiling on an x86 for an arm target."

are you talking about:

  --build=x86_64-linux --host=aarch64-linux

or:
  --host=x86_64-linux --target=aarch64-linux

?

I agree with Andreas -- linux-btrace.c is only included in the build on x86 hosts, so why are you seeing
this?  Did you tweak gdb/configure.nat to include linux-btrace.c in other configurations?  Or gdb/configure.tgt?
What exactly did you do?

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

hi Pedro,

I think I can clarify the situation here by giving more insight on the 
context. A short explanation is needed.

In current build system linux-btrace.c is only included when the host is 
a Linux running of an i386 cpu. (see gdb/configure.nat). so the point of 
Andreas is pertinent with no further explanations of additional planned 
changes.

This patch is a preparation for another patch set for adding btrace on 
arm processors using ARM Coresight ETM traces. (see the patch set 
https://pi.simark.ca/gdb-patches/20210531213307.275079-1-zied.guermazi@trande.de/#r 
). Markus Metzger in his review recommended submitting this change in a 
separated patch.

In the patch set for adding btrace on arm,  there is a need to build 
linux-btrace.c for hosts linux/aarch64 and linux/arm. gdb/configure.nat 
at line 233 looks now as following:

"

/   linux)//
//    case ${gdb_host_cpu} in//
//        aarch64)//
//        #  Host: AArch64 based machine running GNU/Linux//
//        NATDEPFILES="${NATDEPFILES} aarch64-nat.o aarch64-linux-nat.o \//
//        aarch32-linux-nat.o nat/aarch64-hw-point.o \//
//        nat/aarch64-linux-hw-point.o \//
//        nat/aarch64-linux.o \//
//*nat/linux-btrace.o* \//
//        nat/aarch64-sve-linux-ptrace.o \//
//        nat/aarch64-mte-linux-ptrace.o"//
//        ;;//
//        arc)//
//        # Host: ARC based machine running GNU/Linux//
//        NATDEPFILES="${NATDEPFILES} arc-linux-nat.o"//
//        ;;//
//        arm)//
//        # Host: ARM based machine running GNU/Linux//
//        NATDEPFILES="${NATDEPFILES} arm-linux-nat.o \//
//        aarch32-linux-nat.o *nat/linux-btrace.o*"//
//        ;;/

"

which means that the file is now included in the build for linux/aarch64 
and linux/arm


I have observed this issue when the build machine was an intel x86,  
host and target were both aarch64-linux (and also arm-linux)

using following configuration

"../binutils-gdb/configure --host aarch64-linux-gnu 
--target=aarch64-linux-gnu --disable-ld --disable-gold --disable-gas 
--disable-sim --disable-gprofng --with-arm-cs "

and when building natively on Debian on aarch64 and Debian on arm.

I was getting following build error message

"
/../../binutils-gdb/gdb/nat/linux-btrace.c:799:30: error: ‘PAGE_SIZE’ 
was not declared in this scope; did you mean ‘PTRACE_SEIZE’?//
//  799 |   scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | 
PROT_WRITE, MAP_SHARED,//
//      |                              ^~~~~~~~~//
//      |                              PTRACE_SEIZE/

"

The issue was fixed with this patch.


Kind Regards

Zied Guermazi


On 02.05.22 18:46, Pedro Alves wrote:
> On 2022-04-30 14:33, Andreas Schwab wrote:
>> On Apr 30 2022, Zied Guermazi wrote:
>>
>>> PAGE_SIZE for target system was not defined, in other cases PAGE_SIZE of
>>> the host system was used etc... :).
>> This file is only used in a native configuration.
>>
> Zied, when you said:
>
>    "the issue was first observed when activating this code while compiling on an x86 for an arm target."
>
> are you talking about:
>
>    --build=x86_64-linux --host=aarch64-linux
>
> or:
>    --host=x86_64-linux --target=aarch64-linux
>
> ?
>
> I agree with Andreas -- linux-btrace.c is only included in the build on x86 hosts, so why are you seeing
> this?  Did you tweak gdb/configure.nat to include linux-btrace.c in other configurations?  Or gdb/configure.tgt?
> What exactly did you do?

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Pedro Alves]

On 2022-05-02 19:01, Zied Guermazi wrote:
> hi Pedro,
> 
> I think I can clarify the situation here by giving more insight on the context. A short explanation is needed.
> 
> In current build system linux-btrace.c is only included when the host is a Linux running of an i386 cpu. (see gdb/configure.nat).  so the point of Andreas is pertinent with no further explanations of additional planned changes.
> 
> This patch is a preparation for another patch set for adding btrace on arm processors using ARM Coresight ETM traces. (see the patch set https://pi.simark.ca/gdb-patches/20210531213307.275079-1-zied.guermazi@trande.de/#r ). Markus Metzger in his review recommended submitting this change in a separated patch.
> 
> In the patch set for adding btrace on arm,  there is a need to build linux-btrace.c for hosts linux/aarch64 and linux/arm. gdb/configure.nat at line 233 looks now as following:
> 
> "
> 
>  /   linux)//
> //    case ${gdb_host_cpu} in//
> //        aarch64)//
> //        #  Host: AArch64 based machine running GNU/Linux//
> //        NATDEPFILES="${NATDEPFILES} aarch64-nat.o aarch64-linux-nat.o \//
> //        aarch32-linux-nat.o nat/aarch64-hw-point.o \//
> //        nat/aarch64-linux-hw-point.o \//
> //        nat/aarch64-linux.o \//
> //        *nat/linux-btrace.o* \//
> //        nat/aarch64-sve-linux-ptrace.o \//
> //        nat/aarch64-mte-linux-ptrace.o"//
> //        ;;//
> //        arc)//
> //        # Host: ARC based machine running GNU/Linux//
> //        NATDEPFILES="${NATDEPFILES} arc-linux-nat.o"//
> //        ;;//
> //        arm)//
> //        # Host: ARM based machine running GNU/Linux//
> //        NATDEPFILES="${NATDEPFILES} arm-linux-nat.o \//
> //        aarch32-linux-nat.o *nat/linux-btrace.o*"//
> //        ;;/
> 
> "
> 
> which means that the file is now included in the build for linux/aarch64 and linux/arm
> 
> 
> I have observed this issue when the build machine was an intel x86,  host and target were both aarch64-linux (and also arm-linux)

OK, thanks, that clarifies things.  I wasn't aware of that patch series.  The build and target machines (in autoconf terms) are
really irrelevant here.  The only machine that matters here is the host one.

> 
> using following configuration
> 
> "../binutils-gdb/configure --host aarch64-linux-gnu --target=aarch64-linux-gnu --disable-ld --disable-gold --disable-gas --disable-sim --disable-gprofng --with-arm-cs "
> 
> and when building natively on Debian on aarch64 and Debian on arm.

Did you really run configure on one machine, and then typed "make" on a different machine?
Anyhow, doesn't matter.

> 
> I was getting following build error message
> 
> "
> /../../binutils-gdb/gdb/nat/linux-btrace.c:799:30: error: ‘PAGE_SIZE’ was not declared in this scope; did you mean ‘PTRACE_SEIZE’?//
> //  799 |   scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,//
> //      |                              ^~~~~~~~~//
> //      |                              PTRACE_SEIZE/
> 
> "
> 
> The issue was fixed with this patch.

Right.

So in the commit log, instead of:

 "PAGE_SIZE is not defined in all build configurations, e.g cross compiling
 for aarch64 GNU/Linux machine. This patch gets the value at runtime using 
 posix call sysconf (_SC_PAGESIZE)"

don't mention cross compiling at all, as that just confuses things.

I'd suggest instead:

 "Currently, linux-btrace.c is only built when the host is x86 or x86_64 GNU/Linux.
 Later patches will add btrace support on ARM processors using ARM Coresight ETM traces.
 That ran into the fact that PAGE_SIZE is not defined on Aarch64.  This patch fixes that
 by getting the page size value at runtime using POSIX sysconf(_SC_PAGESIZE) instead."

Pedro Alves

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

hi Pedro,

the issue was observed twice: once when building on x86 using

"../binutils-gdb/configure --host aarch64-linux-gnu 
--target=aarch64-linux-gnu --disable-ld --disable-gold --disable-gas 
--disable-sim --disable-gprofng --with-arm-cs"

and once when building natively on Debian on aarch64 by using

"../binutils-gdb/configure --disable-ld --disable-gold --disable-gas 
--disable-sim --disable-gprofng --with-arm-cs"


I will change the comment and re-submit the patch


Kind Regards

Zied Guermazi.

On 02.05.22 20:55, Pedro Alves wrote:
> On 2022-05-02 19:01, Zied Guermazi wrote:
>> hi Pedro,
>>
>> I think I can clarify the situation here by giving more insight on the context. A short explanation is needed.
>>
>> In current build system linux-btrace.c is only included when the host is a Linux running of an i386 cpu. (see gdb/configure.nat).  so the point of Andreas is pertinent with no further explanations of additional planned changes.
>>
>> This patch is a preparation for another patch set for adding btrace on arm processors using ARM Coresight ETM traces. (see the patch sethttps://pi.simark.ca/gdb-patches/20210531213307.275079-1-zied.guermazi@trande.de/#r  ). Markus Metzger in his review recommended submitting this change in a separated patch.
>>
>> In the patch set for adding btrace on arm,  there is a need to build linux-btrace.c for hosts linux/aarch64 and linux/arm. gdb/configure.nat at line 233 looks now as following:
>>
>> "
>>
>>   /   linux)//
>> //    case ${gdb_host_cpu} in//
>> //        aarch64)//
>> //        #  Host: AArch64 based machine running GNU/Linux//
>> //        NATDEPFILES="${NATDEPFILES} aarch64-nat.o aarch64-linux-nat.o \//
>> //        aarch32-linux-nat.o nat/aarch64-hw-point.o \//
>> //        nat/aarch64-linux-hw-point.o \//
>> //        nat/aarch64-linux.o \//
>> //        *nat/linux-btrace.o* \//
>> //        nat/aarch64-sve-linux-ptrace.o \//
>> //        nat/aarch64-mte-linux-ptrace.o"//
>> //        ;;//
>> //        arc)//
>> //        # Host: ARC based machine running GNU/Linux//
>> //        NATDEPFILES="${NATDEPFILES} arc-linux-nat.o"//
>> //        ;;//
>> //        arm)//
>> //        # Host: ARM based machine running GNU/Linux//
>> //        NATDEPFILES="${NATDEPFILES} arm-linux-nat.o \//
>> //        aarch32-linux-nat.o *nat/linux-btrace.o*"//
>> //        ;;/
>>
>> "
>>
>> which means that the file is now included in the build for linux/aarch64 and linux/arm
>>
>>
>> I have observed this issue when the build machine was an intel x86,  host and target were both aarch64-linux (and also arm-linux)
> OK, thanks, that clarifies things.  I wasn't aware of that patch series.  The build and target machines (in autoconf terms) are
> really irrelevant here.  The only machine that matters here is the host one.
>
>> using following configuration
>>
>> "../binutils-gdb/configure --host aarch64-linux-gnu --target=aarch64-linux-gnu --disable-ld --disable-gold --disable-gas --disable-sim --disable-gprofng --with-arm-cs"
>>
>> and when building natively on Debian on aarch64 and Debian on arm.
> Did you really run configure on one machine, and then typed "make" on a different machine?
> Anyhow, doesn't matter.
>
>> I was getting following build error message
>>
>> "
>> /../../binutils-gdb/gdb/nat/linux-btrace.c:799:30: error: ‘PAGE_SIZE’ was not declared in this scope; did you mean ‘PTRACE_SEIZE’?//
>> //  799 |   scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,//
>> //      |                              ^~~~~~~~~//
>> //      |                              PTRACE_SEIZE/
>>
>> "
>>
>> The issue was fixed with this patch.
> Right.
>
> So in the commit log, instead of:
>
>   "PAGE_SIZE is not defined in all build configurations, e.g cross compiling
>   for aarch64 GNU/Linux machine. This patch gets the value at runtime using
>   posix call sysconf (_SC_PAGESIZE)"
>
> don't mention cross compiling at all, as that just confuses things.
>
> I'd suggest instead:
>
>   "Currently, linux-btrace.c is only built when the host is x86 or x86_64 GNU/Linux.
>   Later patches will add btrace support on ARM processors using ARM Coresight ETM traces.
>   That ran into the fact that PAGE_SIZE is not defined on Aarch64.  This patch fixes that
>   by getting the page size value at runtime using POSIX sysconf(_SC_PAGESIZE) instead."
>
> Pedro Alves
-- 

*Zied Guermazi*
founder

Trande GmbH
Leuschnerstraße 2
69469 Weinheim/Germany

Mobile: +491722645127
mailto:zied.guermazi@trande.de <mailto:zied.guermazi@trande.de>

*Trande GmbH*
Leuschnerstraße 2, D-69469 Weinheim; Telefon: +491722645127
Sitz der Gesellschaft: Weinheim- Registergericht: AG Mannheim HRB 736209 
- Geschäftsführung: Zied Guermazi

*Confidentiality Note*
This message is intended only for the use of the named recipient(s) and 
may contain confidential and/or privileged information. If you are not 
the intended recipient, please contact the sender and delete the 
message. Any unauthorized use of the information contained in this 
message is prohibited.

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Tom Tromey]

>>>>> "Zied" == Zied Guermazi <zied.guermazi@trande.de> writes:

Zied> PAGE_SIZE is not defined in all build configurations, e.g cross compiling
Zied> for aarch64 GNU/Linux machine. This patch gets the value at runtime using 
Zied> posix call sysconf (_SC_PAGESIZE)

Thanks.  I think this is ok.

Tom

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Pedro Alves]

On 2022-05-02 17:38, Tom Tromey wrote:
>>>>>> "Zied" == Zied Guermazi <zied.guermazi@trande.de> writes:
> 
> Zied> PAGE_SIZE is not defined in all build configurations, e.g cross compiling
> Zied> for aarch64 GNU/Linux machine. This patch gets the value at runtime using 
> Zied> posix call sysconf (_SC_PAGESIZE)
> 
> Thanks.  I think this is ok.

Please don't merge it yet.  Andreas' question is quite pertinent -- this file in only built on
x86 hosts.  The premise of the patch doesn't seems strange offhand.  Is the Aarch64 port
going to start using linux-btrace.c?  That isn't clear.

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Tom Tromey]

Pedro> Please don't merge it yet.  Andreas' question is quite pertinent
Pedro> -- this file in only built on x86 hosts.  The premise of the
Pedro> patch doesn't seems strange offhand.  Is the Aarch64 port going
Pedro> to start using linux-btrace.c?  That isn't clear.

Based on the thread I assumed it was in preparation for such a merge.

I suppose I don't really know for sure, but on the other hand it also
seemed like this wasn't harmful.

Tom

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Pedro Alves]

On 2022-05-02 18:04, Tom Tromey wrote:
> Pedro> Please don't merge it yet.  Andreas' question is quite pertinent
> Pedro> -- this file in only built on x86 hosts.  The premise of the
> Pedro> patch doesn't seems strange offhand.  Is the Aarch64 port going
> Pedro> to start using linux-btrace.c?  That isn't clear.
> 
> Based on the thread I assumed it was in preparation for such a merge.
> 
> I suppose I don't really know for sure, but on the other hand it also
> seemed like this wasn't harmful.

Yeah, I agree it doesn't seem harmful.  I'd just like to clarify the
motivation, and make sure we have the right info in the commit log.

[PATCH 0/1] btrace: get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

PAGE_SIZE is not defined in all systems, therefore use the posix call sysconf (_SC_PAGESIZE)

Zied Guermazi (1):
  get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

 gdb/nat/linux-btrace.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

---
 gdb/nat/linux-btrace.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index b0d6dcd7cf1..a9aaa9c88b3 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -486,9 +486,10 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
   if (fd.get () < 0)
     diagnose_perf_event_open_fail ();
 
+  long page_size = sysconf (_SC_PAGESIZE);
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = ((size_t) conf->size / PAGE_SIZE
-	   + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1));
+  pages = ((size_t) conf->size / page_size
+	   + ((conf->size % page_size) == 0 ? 0 : 1));
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
@@ -507,17 +508,17 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
       size_t length;
       __u64 data_size;
 
-      data_size = (__u64) pages * PAGE_SIZE;
+      data_size = (__u64) pages * page_size;
 
       /* Don't ask for more than we can represent in the configuration.  */
       if ((__u64) UINT_MAX < data_size)
 	continue;
 
       size = (size_t) data_size;
-      length = size + PAGE_SIZE;
+      length = size + page_size;
 
       /* Check for overflows.  */
-      if ((__u64) length != data_size + PAGE_SIZE)
+      if ((__u64) length != data_size + page_size)
 	continue;
 
       errno = 0;
@@ -532,7 +533,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
 
   struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
     data.get ();
-  data_offset = PAGE_SIZE;
+  data_offset = page_size;
 
 #if defined (PERF_ATTR_SIZE_VER5)
   if (offsetof (struct perf_event_mmap_page, data_size) <= header->size)
@@ -630,7 +631,8 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
     diagnose_perf_event_open_fail ();
 
   /* Allocate the configuration page. */
-  scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+  long page_size = sysconf (_SC_PAGESIZE);
+  scoped_mmap data (nullptr, (size_t) page_size, PROT_READ | PROT_WRITE, MAP_SHARED,
 		    fd.get (), 0);
   if (data.get () == MAP_FAILED)
     error (_("Failed to map trace user page: %s."), safe_strerror (errno));
@@ -641,8 +643,8 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   header->aux_offset = header->data_offset + header->data_size;
 
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = ((size_t) conf->size / PAGE_SIZE
-	   + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1));
+  pages = ((size_t) conf->size / page_size
+	   + ((conf->size % page_size) == 0 ? 0 : 1));
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
@@ -661,7 +663,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
       size_t length;
       __u64 data_size;
 
-      data_size = (__u64) pages * PAGE_SIZE;
+      data_size = (__u64) pages * page_size;
 
       /* Don't ask for more than we can represent in the configuration.  */
       if ((__u64) UINT_MAX < data_size)
@@ -732,7 +734,8 @@ linux_enable_btrace (ptid_t ptid, const struct btrace_config *conf)
 static enum btrace_error
 linux_disable_bts (struct btrace_tinfo_bts *tinfo)
 {
-  munmap((void *) tinfo->header, tinfo->bts.size + PAGE_SIZE);
+  long page_size = sysconf (_SC_PAGESIZE);
+  munmap ((void *) tinfo->header, (size_t) (tinfo->bts.size + page_size));
   close (tinfo->file);
 
   return BTRACE_ERR_NONE;
@@ -743,8 +746,9 @@ linux_disable_bts (struct btrace_tinfo_bts *tinfo)
 static enum btrace_error
 linux_disable_pt (struct btrace_tinfo_pt *tinfo)
 {
-  munmap((void *) tinfo->pt.mem, tinfo->pt.size);
-  munmap((void *) tinfo->header, PAGE_SIZE);
+  long page_size = sysconf (_SC_PAGESIZE);
+  munmap ((void *) tinfo->pt.mem, tinfo->pt.size);
+  munmap ((void *) tinfo->header, (size_t) page_size);
   close (tinfo->file);
 
   return BTRACE_ERR_NONE;
-- 
2.25.1

RE: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Metzger, Markus T]

Hello Zied,

>diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
>index b0d6dcd7cf1..a9aaa9c88b3 100644
>--- a/gdb/nat/linux-btrace.c
>+++ b/gdb/nat/linux-btrace.c
>@@ -486,9 +486,10 @@ linux_enable_bts (ptid_t ptid, const struct
>btrace_config_bts *conf)
>   if (fd.get () < 0)
>     diagnose_perf_event_open_fail ();
>
>+  long page_size = sysconf (_SC_PAGESIZE);

Please check the return value.  You may just error out but we should
detect errors, at least.


>   struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
>     data.get ();
>-  data_offset = PAGE_SIZE;
>+  data_offset = page_size;

Hmmm, I would have expected a compiler warning, here, since PAGE_SIZE is
now long and DATA_OFFSET is __u64.


>   /* Allocate the configuration page. */
>-  scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE,
>MAP_SHARED,
>+  long page_size = sysconf (_SC_PAGESIZE);

Here, we'd also want to check the return value.


>@@ -661,7 +663,7 @@ linux_enable_pt (ptid_t ptid, const struct
>btrace_config_pt *conf)
>       size_t length;
>       __u64 data_size;
>
>-      data_size = (__u64) pages * PAGE_SIZE;
>+      data_size = (__u64) pages * page_size;

Hmmm, this also looks like the compiler would want to warn about
signed and unsigned multiply.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

hi Markus,

thanks for your feedback, please have a look below for the answers.

On 25.04.22 07:03, Metzger, Markus T wrote:
> Hello Zied,
>
>> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
>> index b0d6dcd7cf1..a9aaa9c88b3 100644
>> --- a/gdb/nat/linux-btrace.c
>> +++ b/gdb/nat/linux-btrace.c
>> @@ -486,9 +486,10 @@ linux_enable_bts (ptid_t ptid, const struct
>> btrace_config_bts *conf)
>>    if (fd.get () < 0)
>>      diagnose_perf_event_open_fail ();
>>
>> +  long page_size = sysconf (_SC_PAGESIZE);
> Please check the return value.  You may just error out but we should
> detect errors, at least.
[Zied] according to man pages the returned values should not be below 1. 
I will check against it and write a warning message.
>
>
>>    struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)
>>      data.get ();
>> -  data_offset = PAGE_SIZE;
>> +  data_offset = page_size;
> Hmmm, I would have expected a compiler warning, here, since PAGE_SIZE is
> now long and DATA_OFFSET is __u64.

[Zied] I go a clean compilation on my machine. here is the outcome:

"build_intel/gdb$ make
   CXX    nat/linux-btrace.o
   GEN    init.c
   CXXLD  gdb
"

Do you recommend casting page_size to a __u64?

>
>
>>    /* Allocate the configuration page. */
>> -  scoped_mmap data (nullptr, PAGE_SIZE, PROT_READ | PROT_WRITE,
>> MAP_SHARED,
>> +  long page_size = sysconf (_SC_PAGESIZE);
> Here, we'd also want to check the return value.
[Zied] ok
>
>
>> @@ -661,7 +663,7 @@ linux_enable_pt (ptid_t ptid, const struct
>> btrace_config_pt *conf)
>>        size_t length;
>>        __u64 data_size;
>>
>> -      data_size = (__u64) pages * PAGE_SIZE;
>> +      data_size = (__u64) pages * page_size;
> Hmmm, this also looks like the compiler would want to warn about
> signed and unsigned multiply.

[Zied] No warnings observed. Do you recommend casting page_size to a __u64?

>
> Regards,
> Markus.
>
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0,www.intel.de  <http://www.intel.de>
> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>
-- 
Kind Regards
Zied Guermazi

RE: [PATCH 1/1] get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Metzger, Markus T]

Hello Zied,


Please check the return value.  You may just error out but we should

detect errors, at least.
[Zied] according to man pages the returned values should not be below 1. I will check against it and write a warning message.

I’m referring to: “
On  error,  -1 is returned and errno is set to indicate the cause of the error
“.



  struct perf_event_mmap_page *header = (struct perf_event_mmap_page *)

    data.get ();

-  data_offset = PAGE_SIZE;

+  data_offset = page_size;



Hmmm, I would have expected a compiler warning, here, since PAGE_SIZE is

now long and DATA_OFFSET is __u64.

[Zied] I go a clean compilation on my machine. here is the outcome:

"build_intel/gdb$ make
  CXX    nat/linux-btrace.o
  GEN    init.c
  CXXLD  gdb
"

Do you recommend casting page_size to a __u64?
You may need to enable warnings.    I’m configuring with “
--enable-build-warnings --enable-gdb-build-warnings --enable-werror
“ but I have not checked this case myself.

The original PAGE_SIZE macro is defined as (1UL << PAGE_SHIFT).  After checking for errors, we
should probably cast it to an appropriate unsigned type.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928