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

[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 0/1] btrace: get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Simon Marchi]

On 2022-04-24 17:15, Zied Guermazi wrote:
> PAGE_SIZE is not defined in all systems, therefore use the posix call sysconf (_SC_PAGESIZE)

Hi,

This information should be in the commit message of the patch itself,
not in the cover letter.  Also, can you please give concrete examples of
systems where this isn't defined, to give some context?  It should also
be in the commit message.

Simon

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 0/1] btrace: get page size using sysconf (_SC_PAGESIZE) instead of PAGE_SIZE

[Zied Guermazi]

Hi Simon,

thanks for your feedback, I will put the info in the commit message itself.

the issue was observed when cross compiling for aarch64 GNU/Linux 
machine while activating the code. It was also discussed here: 
https://github.com/intel/libipt/issues/11 .

Kind Regards

Zied Guermazi


On 25.04.22 02:13, Simon Marchi wrote:
> On 2022-04-24 17:15, Zied Guermazi wrote:
>> PAGE_SIZE is not defined in all systems, therefore use the posix call sysconf (_SC_PAGESIZE)
> Hi,
>
> This information should be in the commit message of the patch itself,
> not in the cover letter.  Also, can you please give concrete examples of
> systems where this isn't defined, to give some context?  It should also
> be in the commit message.
>
> Simon
-- 

*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

[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

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

[John Baldwin]

On 4/24/22 11:54 PM, Zied Guermazi wrote:
> Hi Simon,
> 
> thanks for your feedback, I will put the info in the commit message itself.
> 
> the issue was observed when cross compiling for aarch64 GNU/Linux
> machine while activating the code. It was also discussed here:
> https://github.com/intel/libipt/issues/11 .

FWIW, FreeBSD/aarch64 is also a system (in future releases) where PAGE_SIZE
won't be defined to support 4k vs 16k page sizes being a dynamic, run-time
decision (at least for the userland ABI).

-- 
John Baldwin