* [patch, avr] Fix incorrect argument register for push dummy call @ 2016-02-25 12:35 Pitchumani Sivanupandi 2016-02-26 14:15 ` Luis Machado 0 siblings, 1 reply; 3+ messages in thread From: Pitchumani Sivanupandi @ 2016-02-25 12:35 UTC (permalink / raw) To: gdb-patches [-- Attachment #1: Type: text/plain, Size: 631 bytes --] There are few failures for avr-gdb in callfuncs.exp. These failures are due to the incorrect argument passing in case of push dummy call. The last argument register is calculated incorrectly that caused this issue. Attached patch fixes the calculation so that the push dummy call aligns with the code generated by avr-gcc. No new regressions found when tested with internal simulator. If ok, could someone commit please? Regards, Pitchumani gdb/ChangeLog 2016-02-25 Pitchumani Sivanupandi <pitchumani.s@atmel.com> * avr-tdep.c (AVR_LAST_ARG_REGNUM): Define. (avr_push_dummy_call): Correct last needed argument register. [-- Attachment #2: avr-last-argument-register-fix.patch --] [-- Type: text/plain, Size: 1308 bytes --] diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c index 597cfb4..18ecfe4 100644 --- a/gdb/avr-tdep.c +++ b/gdb/avr-tdep.c @@ -111,6 +111,7 @@ enum AVR_ARG1_REGNUM = 24, /* Single byte argument */ AVR_ARGN_REGNUM = 25, /* Multi byte argments */ + AVR_LAST_ARG_REGNUM = 8, /* Last argument register */ AVR_RET1_REGNUM = 24, /* Single byte return value */ AVR_RETN_REGNUM = 25, /* Multi byte return value */ @@ -1298,12 +1299,14 @@ avr_push_dummy_call (struct gdbarch *gdbarch, struct value *function, const bfd_byte *contents = value_contents (arg); int len = TYPE_LENGTH (type); - /* Calculate the potential last register needed. */ - last_regnum = regnum - (len + (len & 1)); + /* Calculate the potential last register needed. + E.g. For length 2, registers regnum and regnum-1 (say 25 and 24) + shall be used. So, last needed register will be regnum-1(24). */ + last_regnum = regnum - (len + (len & 1)) + 1; /* If there are registers available, use them. Once we start putting stuff on the stack, all subsequent args go on stack. */ - if ((si == NULL) && (last_regnum >= 8)) + if ((si == NULL) && (last_regnum >= AVR_LAST_ARG_REGNUM)) { ULONGEST val; ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch, avr] Fix incorrect argument register for push dummy call 2016-02-25 12:35 [patch, avr] Fix incorrect argument register for push dummy call Pitchumani Sivanupandi @ 2016-02-26 14:15 ` Luis Machado 2016-02-29 6:25 ` Sivanupandi, Pitchumani 0 siblings, 1 reply; 3+ messages in thread From: Luis Machado @ 2016-02-26 14:15 UTC (permalink / raw) To: Pitchumani Sivanupandi, gdb-patches Hi, On 02/25/2016 09:34 AM, Pitchumani Sivanupandi wrote: > There are few failures for avr-gdb in callfuncs.exp. These failures are > due to the incorrect argument passing in case of push dummy call. The > last argument register is calculated incorrectly that caused this issue. > > Attached patch fixes the calculation so that the push dummy call aligns > with the code generated by avr-gcc. > > No new regressions found when tested with internal simulator. > > If ok, could someone commit please? > > Regards, > Pitchumani > > gdb/ChangeLog > > 2016-02-25 Pitchumani Sivanupandi<pitchumani.s@atmel.com> > > * avr-tdep.c (AVR_LAST_ARG_REGNUM): Define. > (avr_push_dummy_call): Correct last needed argument register. > > > avr-last-argument-register-fix.patch > > > diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c > index 597cfb4..18ecfe4 100644 > --- a/gdb/avr-tdep.c > +++ b/gdb/avr-tdep.c > @@ -111,6 +111,7 @@ enum > > AVR_ARG1_REGNUM = 24, /* Single byte argument */ > AVR_ARGN_REGNUM = 25, /* Multi byte argments */ > + AVR_LAST_ARG_REGNUM = 8, /* Last argument register */ > > AVR_RET1_REGNUM = 24, /* Single byte return value */ > AVR_RETN_REGNUM = 25, /* Multi byte return value */ > @@ -1298,12 +1299,14 @@ avr_push_dummy_call (struct gdbarch *gdbarch, struct value *function, > const bfd_byte *contents = value_contents (arg); > int len = TYPE_LENGTH (type); > > - /* Calculate the potential last register needed. */ > - last_regnum = regnum - (len + (len & 1)); > + /* Calculate the potential last register needed. > + E.g. For length 2, registers regnum and regnum-1 (say 25 and 24) Is that how the code works? It seems a bit confusing, because we want to start in even-aligned registers, so we'd use r25:r24, r23:r22 etc. So last_regnum always ends up pointing at an odd-numbered register, so the code can decrement the odd-numbered register if needed (in case we have data of length 1/3/...). /* Skip a register for odd length args. */ if (len & 1) regnum--; > + shall be used. So, last needed register will be regnum-1(24). */ > + last_regnum = regnum - (len + (len & 1)) + 1; > Isn't this going to be incorrect for, say, data of length 3? last_regnum = 25 - (3 + (3 & 1)) + 1 -> 25 - (3 + 1) + 1 -> 22 But it should've been 21, because data would occupy registers 25:24 and 23:22 and then we'd need an even-aligned register to start with? > /* If there are registers available, use them. Once we start putting > stuff on the stack, all subsequent args go on stack. */ > - if ((si == NULL) && (last_regnum >= 8)) > + if ((si == NULL) && (last_regnum >= AVR_LAST_ARG_REGNUM)) > { > ULONGEST val; > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch, avr] Fix incorrect argument register for push dummy call 2016-02-26 14:15 ` Luis Machado @ 2016-02-29 6:25 ` Sivanupandi, Pitchumani 0 siblings, 0 replies; 3+ messages in thread From: Sivanupandi, Pitchumani @ 2016-02-29 6:25 UTC (permalink / raw) To: Luis Machado; +Cc: gdb-patches On 26-02-2016 19:45, Luis Machado wrote: > Hi, > > On 02/25/2016 09:34 AM, Pitchumani Sivanupandi wrote: >> There are few failures for avr-gdb in callfuncs.exp. These failures are >> due to the incorrect argument passing in case of push dummy call. The >> last argument register is calculated incorrectly that caused this issue. >> >> Attached patch fixes the calculation so that the push dummy call aligns >> with the code generated by avr-gcc. >> >> No new regressions found when tested with internal simulator. >> >> If ok, could someone commit please? >> >> Regards, >> Pitchumani >> >> gdb/ChangeLog >> >> 2016-02-25 Pitchumani Sivanupandi<pitchumani.s@atmel.com> >> >> * avr-tdep.c (AVR_LAST_ARG_REGNUM): Define. >> (avr_push_dummy_call): Correct last needed argument register. >> >> >> avr-last-argument-register-fix.patch >> >> >> diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c >> index 597cfb4..18ecfe4 100644 >> --- a/gdb/avr-tdep.c >> +++ b/gdb/avr-tdep.c >> @@ -111,6 +111,7 @@ enum >> >> AVR_ARG1_REGNUM = 24, /* Single byte argument */ >> AVR_ARGN_REGNUM = 25, /* Multi byte argments */ >> + AVR_LAST_ARG_REGNUM = 8, /* Last argument register */ >> >> AVR_RET1_REGNUM = 24, /* Single byte return value */ >> AVR_RETN_REGNUM = 25, /* Multi byte return value */ >> @@ -1298,12 +1299,14 @@ avr_push_dummy_call (struct gdbarch *gdbarch, >> struct value *function, >> const bfd_byte *contents = value_contents (arg); >> int len = TYPE_LENGTH (type); >> >> - /* Calculate the potential last register needed. */ >> - last_regnum = regnum - (len + (len & 1)); >> + /* Calculate the potential last register needed. >> + E.g. For length 2, registers regnum and regnum-1 (say 25 and >> 24) > > Is that how the code works? It seems a bit confusing, because we want to > start in even-aligned registers, so we'd use r25:r24, r23:r22 etc. Yes. Lowest even register will be loaded with LSB of argument, and subsequent bytes passed in subsequent registers (i.e. in increasing register number). > So last_regnum always ends up pointing at an odd-numbered register, so > the code can decrement the odd-numbered register if needed (in case we > have data of length 1/3/...). > > /* Skip a register for odd length args. */ > if (len & 1) > regnum--; No, last_regnum should point at last even register where LSB of argument should be loaded. Variable 'regnum' should point to register which loads MSB of argument. In case of odd length argument, regnum-1 will hold the MSB of argument. So, it is decremented. Ref: https://gcc.gnu.org/wiki/avr-gcc#Calling_Convention >> + shall be used. So, last needed register will be >> regnum-1(24). */ >> + last_regnum = regnum - (len + (len & 1)) + 1; >> > > Isn't this going to be incorrect for, say, data of length 3? > > last_regnum = 25 - (3 + (3 & 1)) + 1 -> 25 - (3 + 1) + 1 -> 22 > > But it should've been 21, because data would occupy registers 25:24 and > 23:22 and then we'd need an even-aligned register to start with? Variable last_regnum indicates the end of register that required to pass the argument. In case of length 3, we need 22 (LSB), 23 and 24(MSB) to pass the argument. So, last register required is 22, not 21. Regards, Pitchumani ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-02-29 6:25 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-25 12:35 [patch, avr] Fix incorrect argument register for push dummy call Pitchumani Sivanupandi 2016-02-26 14:15 ` Luis Machado 2016-02-29 6:25 ` Sivanupandi, Pitchumani
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox