* [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules. @ 2021-05-11 13:20 Luis Machado 2021-05-13 10:50 ` Alan Hayward 2021-05-13 10:50 ` Alan Hayward 0 siblings, 2 replies; 8+ messages in thread From: Luis Machado @ 2021-05-11 13:20 UTC (permalink / raw) To: gdb-patches When we want to fetch tags from a memory range, the last address in that range is not included. There is a off-by-one error in aarch64_mte_get_tag_granules, which this patch fixes. gdb/ChangeLog: YYYY-MM-DD Luis Machado <luis.machado@linaro.org> * arch/aarch64-mte-linux.c (aarch64_mte_get_tag_granules): Don't include the last address in the range. --- gdb/arch/aarch64-mte-linux.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gdb/arch/aarch64-mte-linux.c b/gdb/arch/aarch64-mte-linux.c index 959c0247ed5..7c2ae9a7058 100644 --- a/gdb/arch/aarch64-mte-linux.c +++ b/gdb/arch/aarch64-mte-linux.c @@ -31,9 +31,10 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size) /* Start address */ CORE_ADDR s_addr = align_down (addr, granule_size); /* End address */ - CORE_ADDR e_addr = align_down (addr + len, granule_size); + CORE_ADDR e_addr = align_down (addr + len - 1, granule_size); - /* We always have at least 1 granule. */ + /* We always have at least 1 granule because len is non-zero at this + point. */ return 1 + (e_addr - s_addr) / granule_size; } -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules. 2021-05-11 13:20 [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules Luis Machado @ 2021-05-13 10:50 ` Alan Hayward 2021-05-13 10:56 ` Luis Machado 2021-05-13 10:50 ` Alan Hayward 1 sibling, 1 reply; 8+ messages in thread From: Alan Hayward @ 2021-05-13 10:50 UTC (permalink / raw) To: Luis Machado; +Cc: gdb-patches\@sourceware.org, nd > On 11 May 2021, at 14:20, Luis Machado via Gdb-patches <gdb-patches@sourceware.org> wrote: > > When we want to fetch tags from a memory range, the last address in that > range is not included. > > There is a off-by-one error in aarch64_mte_get_tag_granules, which this > patch fixes. > > gdb/ChangeLog: > > YYYY-MM-DD Luis Machado <luis.machado@linaro.org> > > * arch/aarch64-mte-linux.c (aarch64_mte_get_tag_granules): Don't > include the last address in the range. > --- > gdb/arch/aarch64-mte-linux.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/gdb/arch/aarch64-mte-linux.c b/gdb/arch/aarch64-mte-linux.c > index 959c0247ed5..7c2ae9a7058 100644 > --- a/gdb/arch/aarch64-mte-linux.c > +++ b/gdb/arch/aarch64-mte-linux.c > @@ -31,9 +31,10 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size) > /* Start address */ > CORE_ADDR s_addr = align_down (addr, granule_size); > /* End address */ > - CORE_ADDR e_addr = align_down (addr + len, granule_size); > + CORE_ADDR e_addr = align_down (addr + len - 1, granule_size); > > - /* We always have at least 1 granule. */ > + /* We always have at least 1 granule because len is non-zero at this > + point. */ > return 1 + (e_addr - s_addr) / granule_size; Ok, I’m happy with round down because we never want to read/write a partial tag But if len is greater than zero, but less than the size of a one tag, then it returns 1. Is that correct? Should it instead be: if (len < granule_size) return 0; > } > > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules. 2021-05-13 10:50 ` Alan Hayward @ 2021-05-13 10:56 ` Luis Machado 2021-05-13 11:24 ` Alan Hayward 0 siblings, 1 reply; 8+ messages in thread From: Luis Machado @ 2021-05-13 10:56 UTC (permalink / raw) To: Alan Hayward; +Cc: gdb-patches\@sourceware.org, nd On 5/13/21 7:50 AM, Alan Hayward wrote: > > >> On 11 May 2021, at 14:20, Luis Machado via Gdb-patches <gdb-patches@sourceware.org> wrote: >> >> When we want to fetch tags from a memory range, the last address in that >> range is not included. >> >> There is a off-by-one error in aarch64_mte_get_tag_granules, which this >> patch fixes. >> >> gdb/ChangeLog: >> >> YYYY-MM-DD Luis Machado <luis.machado@linaro.org> >> >> * arch/aarch64-mte-linux.c (aarch64_mte_get_tag_granules): Don't >> include the last address in the range. >> --- >> gdb/arch/aarch64-mte-linux.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/gdb/arch/aarch64-mte-linux.c b/gdb/arch/aarch64-mte-linux.c >> index 959c0247ed5..7c2ae9a7058 100644 >> --- a/gdb/arch/aarch64-mte-linux.c >> +++ b/gdb/arch/aarch64-mte-linux.c >> @@ -31,9 +31,10 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size) >> /* Start address */ >> CORE_ADDR s_addr = align_down (addr, granule_size); >> /* End address */ >> - CORE_ADDR e_addr = align_down (addr + len, granule_size); >> + CORE_ADDR e_addr = align_down (addr + len - 1, granule_size); >> >> - /* We always have at least 1 granule. */ >> + /* We always have at least 1 granule because len is non-zero at this >> + point. */ >> return 1 + (e_addr - s_addr) / granule_size; > > Ok, I’m happy with round down because we never want to read/write a partial tag > > But if len is greater than zero, but less than the size of a one tag, then it returns 1. Is that correct? The range is specified by [address, address + len). If len is non-zero, we will always have a non-empty range and therefore will always have tag granules. > Should it instead be: > if (len < granule_size) > return 0; We want to know how many tag granules exist in a particular range. We only get 0 granules if the range is empty. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules. 2021-05-13 10:56 ` Luis Machado @ 2021-05-13 11:24 ` Alan Hayward 2021-05-13 11:50 ` Luis Machado 0 siblings, 1 reply; 8+ messages in thread From: Alan Hayward @ 2021-05-13 11:24 UTC (permalink / raw) To: Luis Machado; +Cc: gdb-patches\@sourceware.org, nd On 13 May 2021, at 11:56, Luis Machado <luis.machado@linaro.org<mailto:luis.machado@linaro.org>> wrote: On 5/13/21 7:50 AM, Alan Hayward wrote: On 11 May 2021, at 14:20, Luis Machado via Gdb-patches <gdb-patches@sourceware.org<mailto:gdb-patches@sourceware.org>> wrote: When we want to fetch tags from a memory range, the last address in that range is not included. There is a off-by-one error in aarch64_mte_get_tag_granules, which this patch fixes. gdb/ChangeLog: YYYY-MM-DD Luis Machado <luis.machado@linaro.org<mailto:luis.machado@linaro.org>> * arch/aarch64-mte-linux.c (aarch64_mte_get_tag_granules): Don't include the last address in the range. --- gdb/arch/aarch64-mte-linux.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gdb/arch/aarch64-mte-linux.c b/gdb/arch/aarch64-mte-linux.c index 959c0247ed5..7c2ae9a7058 100644 --- a/gdb/arch/aarch64-mte-linux.c +++ b/gdb/arch/aarch64-mte-linux.c @@ -31,9 +31,10 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size) /* Start address */ CORE_ADDR s_addr = align_down (addr, granule_size); /* End address */ - CORE_ADDR e_addr = align_down (addr + len, granule_size); + CORE_ADDR e_addr = align_down (addr + len - 1, granule_size); - /* We always have at least 1 granule. */ + /* We always have at least 1 granule because len is non-zero at this + point. */ return 1 + (e_addr - s_addr) / granule_size; Ok, I’m happy with round down because we never want to read/write a partial tag But if len is greater than zero, but less than the size of a one tag, then it returns 1. Is that correct? The range is specified by [address, address + len). If len is non-zero, we will always have a non-empty range and therefore will always have tag granules. Should it instead be: if (len < granule_size) return 0; We want to know how many tag granules exist in a particular range. We only get 0 granules if the range is empty. Assuming addr is aligned, then calculating length -> return value 3 * granulesize -> 3 2.5 * granulesize -> 2 2 * granulesize -> 2 1.5 * granulesize -> 1 1 * granulesize -> 1 0.5 * granulesize -> 1. - this doesn’t match the others. 0 -> 0 ? Alan. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules. 2021-05-13 11:24 ` Alan Hayward @ 2021-05-13 11:50 ` Luis Machado 2021-05-13 12:59 ` Alan Hayward 0 siblings, 1 reply; 8+ messages in thread From: Luis Machado @ 2021-05-13 11:50 UTC (permalink / raw) To: Alan Hayward; +Cc: gdb-patches\@sourceware.org, nd On 5/13/21 8:24 AM, Alan Hayward wrote: > > >> On 13 May 2021, at 11:56, Luis Machado <luis.machado@linaro.org >> <mailto:luis.machado@linaro.org>> wrote: >> >> On 5/13/21 7:50 AM, Alan Hayward wrote: >>>> On 11 May 2021, at 14:20, Luis Machado via Gdb-patches >>>> <gdb-patches@sourceware.org <mailto:gdb-patches@sourceware.org>> wrote: >>>> >>>> When we want to fetch tags from a memory range, the last address in that >>>> range is not included. >>>> >>>> There is a off-by-one error in aarch64_mte_get_tag_granules, which this >>>> patch fixes. >>>> >>>> gdb/ChangeLog: >>>> >>>> YYYY-MM-DD Luis Machado <luis.machado@linaro.org >>>> <mailto:luis.machado@linaro.org>> >>>> >>>> * arch/aarch64-mte-linux.c (aarch64_mte_get_tag_granules): Don't >>>> include the last address in the range. >>>> --- >>>> gdb/arch/aarch64-mte-linux.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/gdb/arch/aarch64-mte-linux.c b/gdb/arch/aarch64-mte-linux.c >>>> index 959c0247ed5..7c2ae9a7058 100644 >>>> --- a/gdb/arch/aarch64-mte-linux.c >>>> +++ b/gdb/arch/aarch64-mte-linux.c >>>> @@ -31,9 +31,10 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, >>>> size_t len, size_t granule_size) >>>> /* Start address */ >>>> CORE_ADDR s_addr = align_down (addr, granule_size); >>>> /* End address */ >>>> - CORE_ADDR e_addr = align_down (addr + len, granule_size); >>>> + CORE_ADDR e_addr = align_down (addr + len - 1, granule_size); >>>> >>>> - /* We always have at least 1 granule. */ >>>> + /* We always have at least 1 granule because len is non-zero at this >>>> + point. */ >>>> return 1 + (e_addr - s_addr) / granule_size; >>> Ok, I’m happy with round down because we never want to read/write a >>> partial tag >>> But if len is greater than zero, but less than the size of a one tag, >>> then it returns 1. Is that correct? >> >> The range is specified by [address, address + len). If len is >> non-zero, we will always have a non-empty range and therefore will >> always have tag granules. >> >>> Should it instead be: >>> if (len < granule_size) >>> return 0; >> >> We want to know how many tag granules exist in a particular range. We >> only get 0 granules if the range is empty. > > Assuming addr is aligned, then calculating length -> return value > 3 * granulesize -> 3 > 2.5 * granulesize -> 2 > 2 * granulesize -> 2 > 1.5 * granulesize -> 1 > 1 * granulesize -> 1 > 0.5 * granulesize -> 1. - this doesn’t match the others. > 0 -> 0 Sorry. I'm not following. Assuming addr is aligned to the granule size, we'd have the following length -> granules mapping: 0 -> 0 1 -> 1 granule 2 -> 1 granule 3 -> 1 granule ... 15 -> 1 granule 16 -> 2 granules 17 -> 2 granules ... and so on. > > ? > > Alan. > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules. 2021-05-13 11:50 ` Luis Machado @ 2021-05-13 12:59 ` Alan Hayward 2021-05-13 13:15 ` Luis Machado 0 siblings, 1 reply; 8+ messages in thread From: Alan Hayward @ 2021-05-13 12:59 UTC (permalink / raw) To: Luis Machado; +Cc: gdb-patches\@sourceware.org, nd On 13 May 2021, at 12:50, Luis Machado <luis.machado@linaro.org<mailto:luis.machado@linaro.org>> wrote: On 5/13/21 8:24 AM, Alan Hayward wrote: On 13 May 2021, at 11:56, Luis Machado <luis.machado@linaro.org<mailto:luis.machado@linaro.org><mailto:luis.machado@linaro.org>> wrote: On 5/13/21 7:50 AM, Alan Hayward wrote: On 11 May 2021, at 14:20, Luis Machado via Gdb-patches <gdb-patches@sourceware.org<mailto:gdb-patches@sourceware.org> <mailto:gdb-patches@sourceware.org>> wrote: When we want to fetch tags from a memory range, the last address in that range is not included. There is a off-by-one error in aarch64_mte_get_tag_granules, which this patch fixes. gdb/ChangeLog: YYYY-MM-DD Luis Machado <luis.machado@linaro.org<mailto:luis.machado@linaro.org><mailto:luis.machado@linaro.org>> * arch/aarch64-mte-linux.c (aarch64_mte_get_tag_granules): Don't include the last address in the range. --- gdb/arch/aarch64-mte-linux.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gdb/arch/aarch64-mte-linux.c b/gdb/arch/aarch64-mte-linux.c index 959c0247ed5..7c2ae9a7058 100644 --- a/gdb/arch/aarch64-mte-linux.c +++ b/gdb/arch/aarch64-mte-linux.c @@ -31,9 +31,10 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size) /* Start address */ CORE_ADDR s_addr = align_down (addr, granule_size); /* End address */ - CORE_ADDR e_addr = align_down (addr + len, granule_size); + CORE_ADDR e_addr = align_down (addr + len - 1, granule_size); - /* We always have at least 1 granule. */ + /* We always have at least 1 granule because len is non-zero at this + point. */ return 1 + (e_addr - s_addr) / granule_size; Ok, I’m happy with round down because we never want to read/write a partial tag But if len is greater than zero, but less than the size of a one tag, then it returns 1. Is that correct? The range is specified by [address, address + len). If len is non-zero, we will always have a non-empty range and therefore will always have tag granules. Should it instead be: if (len < granule_size) return 0; We want to know how many tag granules exist in a particular range. We only get 0 granules if the range is empty. Assuming addr is aligned, then calculating length -> return value > 3 * granulesize -> 3 2.5 * granulesize -> 2 2 * granulesize -> 2 1.5 * granulesize -> 1 1 * granulesize -> 1 0.5 * granulesize -> 1. - this doesn’t match the others. 0 -> 0 Sorry. I'm not following. Assuming addr is aligned to the granule size, we'd have the following length -> granules mapping: 0 -> 0 1 -> 1 granule 2 -> 1 granule 3 -> 1 granule ... 15 -> 1 granule 16 -> 2 granules 17 -> 2 granules ... and so on. Right, I was getting confused. It's rounding up to the next whole granule (whilst ensuring min value of 1). Need more coffee. Ok, looks good to me :) Alan. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules. 2021-05-13 12:59 ` Alan Hayward @ 2021-05-13 13:15 ` Luis Machado 0 siblings, 0 replies; 8+ messages in thread From: Luis Machado @ 2021-05-13 13:15 UTC (permalink / raw) To: Alan Hayward; +Cc: gdb-patches\@sourceware.org, nd On 5/13/21 9:59 AM, Alan Hayward wrote: > > >> On 13 May 2021, at 12:50, Luis Machado <luis.machado@linaro.org >> <mailto:luis.machado@linaro.org>> wrote: >> >> On 5/13/21 8:24 AM, Alan Hayward wrote: >>>> On 13 May 2021, at 11:56, Luis Machado <luis.machado@linaro.org >>>> <mailto:luis.machado@linaro.org><mailto:luis.machado@linaro.org >>>> <mailto:luis.machado@linaro.org>>> wrote: >>>> >>>> On 5/13/21 7:50 AM, Alan Hayward wrote: >>>>>> On 11 May 2021, at 14:20, Luis Machado via Gdb-patches >>>>>> <gdb-patches@sourceware.org >>>>>> <mailto:gdb-patches@sourceware.org><mailto:gdb-patches@sourceware.org >>>>>> <mailto:gdb-patches@sourceware.org>>> wrote: >>>>>> >>>>>> When we want to fetch tags from a memory range, the last address >>>>>> in that >>>>>> range is not included. >>>>>> >>>>>> There is a off-by-one error in aarch64_mte_get_tag_granules, which >>>>>> this >>>>>> patch fixes. >>>>>> >>>>>> gdb/ChangeLog: >>>>>> >>>>>> YYYY-MM-DD Luis Machado <luis.machado@linaro.org >>>>>> <mailto:luis.machado@linaro.org><mailto:luis.machado@linaro.org >>>>>> <mailto:luis.machado@linaro.org>>> >>>>>> >>>>>> * arch/aarch64-mte-linux.c (aarch64_mte_get_tag_granules): Don't >>>>>> include the last address in the range. >>>>>> --- >>>>>> gdb/arch/aarch64-mte-linux.c | 5 +++-- >>>>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/gdb/arch/aarch64-mte-linux.c >>>>>> b/gdb/arch/aarch64-mte-linux.c >>>>>> index 959c0247ed5..7c2ae9a7058 100644 >>>>>> --- a/gdb/arch/aarch64-mte-linux.c >>>>>> +++ b/gdb/arch/aarch64-mte-linux.c >>>>>> @@ -31,9 +31,10 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, >>>>>> size_t len, size_t granule_size) >>>>>> /* Start address */ >>>>>> CORE_ADDR s_addr = align_down (addr, granule_size); >>>>>> /* End address */ >>>>>> - CORE_ADDR e_addr = align_down (addr + len, granule_size); >>>>>> + CORE_ADDR e_addr = align_down (addr + len - 1, granule_size); >>>>>> >>>>>> - /* We always have at least 1 granule. */ >>>>>> + /* We always have at least 1 granule because len is non-zero at >>>>>> this >>>>>> + point. */ >>>>>> return 1 + (e_addr - s_addr) / granule_size; >>>>> Ok, I’m happy with round down because we never want to read/write >>>>> a partial tag >>>>> But if len is greater than zero, but less than the size of a one >>>>> tag, then it returns 1. Is that correct? >>>> >>>> The range is specified by [address, address + len). If len is >>>> non-zero, we will always have a non-empty range and therefore will >>>> always have tag granules. >>>> >>>>> Should it instead be: >>>>> if (len < granule_size) >>>>> return 0; >>>> >>>> We want to know how many tag granules exist in a particular range. >>>> We only get 0 granules if the range is empty. >>> Assuming addr is aligned, then calculating length -> return value > 3 >>> * granulesize -> 3 >>> 2.5 * granulesize -> 2 >>> 2 * granulesize -> 2 >>> 1.5 * granulesize -> 1 >>> 1 * granulesize -> 1 >>> 0.5 * granulesize -> 1. - this doesn’t match the others. >>> 0 -> 0 >> >> Sorry. I'm not following. Assuming addr is aligned to the granule >> size, we'd have the following length -> granules mapping: >> >> 0 -> 0 >> 1 -> 1 granule >> 2 -> 1 granule >> 3 -> 1 granule >> ... >> 15 -> 1 granule >> 16 -> 2 granules >> 17 -> 2 granules >> ... and so on. >> > > Right, I was getting confused. It's rounding up to the next whole > granule (whilst ensuring min value of 1). Need more coffee. > > Ok, looks good to me :) Thanks! :-) Pushed now. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules. 2021-05-11 13:20 [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules Luis Machado 2021-05-13 10:50 ` Alan Hayward @ 2021-05-13 10:50 ` Alan Hayward 1 sibling, 0 replies; 8+ messages in thread From: Alan Hayward @ 2021-05-13 10:50 UTC (permalink / raw) To: Luis Machado; +Cc: gdb-patches\@sourceware.org, nd On 11 May 2021, at 14:20, Luis Machado via Gdb-patches <gdb-patches@sourceware.org<mailto:gdb-patches@sourceware.org>> wrote: When we want to fetch tags from a memory range, the last address in that range is not included. There is a off-by-one error in aarch64_mte_get_tag_granules, which this patch fixes. gdb/ChangeLog: YYYY-MM-DD Luis Machado <luis.machado@linaro.org<mailto:luis.machado@linaro.org>> * arch/aarch64-mte-linux.c (aarch64_mte_get_tag_granules): Don't include the last address in the range. --- gdb/arch/aarch64-mte-linux.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gdb/arch/aarch64-mte-linux.c b/gdb/arch/aarch64-mte-linux.c index 959c0247ed5..7c2ae9a7058 100644 --- a/gdb/arch/aarch64-mte-linux.c +++ b/gdb/arch/aarch64-mte-linux.c @@ -31,9 +31,10 @@ aarch64_mte_get_tag_granules (CORE_ADDR addr, size_t len, size_t granule_size) /* Start address */ CORE_ADDR s_addr = align_down (addr, granule_size); /* End address */ - CORE_ADDR e_addr = align_down (addr + len, granule_size); + CORE_ADDR e_addr = align_down (addr + len - 1, granule_size); - /* We always have at least 1 granule. */ + /* We always have at least 1 granule because len is non-zero at this + point. */ return 1 + (e_addr - s_addr) / granule_size; Ok, I’m happy with round down because we never want to read/write a partial tag But if len is greater than zero, but less than the size of a one tag, then it returns 1. Is that correct? Should it instead be: if (len < granule_size) return 0; } -- 2.25.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-05-13 13:15 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-11 13:20 [PATCH] [GDB,AArch64] Fix off-by-one when calculating tag granules Luis Machado 2021-05-13 10:50 ` Alan Hayward 2021-05-13 10:56 ` Luis Machado 2021-05-13 11:24 ` Alan Hayward 2021-05-13 11:50 ` Luis Machado 2021-05-13 12:59 ` Alan Hayward 2021-05-13 13:15 ` Luis Machado 2021-05-13 10:50 ` Alan Hayward
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).