* Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64
[not found] <gerrit.1571043259000.Id55c74755f0a431ce31223acc86865718ae0c123@gnutoolchain-gerrit.osci.io>
@ 2019-10-15 4:54 ` Simon Marchi (Code Review)
2019-10-15 9:11 ` Tom de Vries (Code Review)
` (4 subsequent siblings)
5 siblings, 0 replies; 6+ messages in thread
From: Simon Marchi (Code Review) @ 2019-10-15 4:54 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
Simon Marchi has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................
Patch Set 1:
(1 comment)
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c
File gdb/amd64-tdep.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c@645
PS1, Line 645: theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
If we reach here, isn't subclass[1] always NO_CLASS? So this if would be unnecessary?
--
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings
^ permalink raw reply [flat|nested] 6+ messages in thread* Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64
[not found] <gerrit.1571043259000.Id55c74755f0a431ce31223acc86865718ae0c123@gnutoolchain-gerrit.osci.io>
2019-10-15 4:54 ` Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64 Simon Marchi (Code Review)
@ 2019-10-15 9:11 ` Tom de Vries (Code Review)
2019-10-15 12:46 ` Simon Marchi (Code Review)
` (3 subsequent siblings)
5 siblings, 0 replies; 6+ messages in thread
From: Tom de Vries (Code Review) @ 2019-10-15 9:11 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................
Patch Set 1:
(1 comment)
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c
File gdb/amd64-tdep.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c@645
PS1, Line 645: theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
> If we reach here, isn't subclass[1] always NO_CLASS? So this if would be unnecessary?
Doing:
...
if (pos == 0)
- theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
+ {
+ gdb_assert (subclass[1] == AMD64_NO_CLASS);
+ theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
+ }
...
gives 180 failure in gdb.base/infcall-nested-structs.exp.
The assert is triggered for the cases where amd64_classify sets theclass[1], in this test-case:
- long double
- double _Complex.
Structurewise, we have (leaving out the massive comment that makes it hard to read):
...
theclass[pos] = amd64_merge_classes (theclass[pos], subclass[0]);
if (bitsize <= 64 && pos == 0 && endpos == 1)
theclass[1] = amd64_merge_classes (theclass[1], subclass[0]);
if (pos == 0)
theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
...
so 'reach here' means just pos == 0.
This would actually be more precise:
...
- if (pos == 0)
+ if (pos == 0 && endpos == 1)
theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
...
and therefore we could restructure to:
...
theclass[pos] = amd64_merge_classes (theclass[pos], subclass[0]);
if (pos == 0 && endpos == 1)
{
theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
if (bitsize <= 64)
theclass[1] = amd64_merge_classes (theclass[1], subclass[0]);
}
...
But I don't think we want to do refactoring like this in this commit.
--
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings
^ permalink raw reply [flat|nested] 6+ messages in thread* Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64
[not found] <gerrit.1571043259000.Id55c74755f0a431ce31223acc86865718ae0c123@gnutoolchain-gerrit.osci.io>
2019-10-15 4:54 ` Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64 Simon Marchi (Code Review)
2019-10-15 9:11 ` Tom de Vries (Code Review)
@ 2019-10-15 12:46 ` Simon Marchi (Code Review)
2019-10-15 13:05 ` Andrew Burgess (Code Review)
` (2 subsequent siblings)
5 siblings, 0 replies; 6+ messages in thread
From: Simon Marchi (Code Review) @ 2019-10-15 12:46 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
Simon Marchi has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................
Patch Set 1:
(1 comment)
So, it LGTM, but is there anybody else that you think should review it? Andrew or Alan perhaps?
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c
File gdb/amd64-tdep.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c@645
PS1, Line 645: theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
> Doing: […]
Ah ok that makes it clear. I thought that non-aggregate fields would always just use the first eightbyte, therefore just set subclass[0] and leave subclass[1] untouched. But that's not the case, so nevermind.
--
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings
^ permalink raw reply [flat|nested] 6+ messages in thread* Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64
[not found] <gerrit.1571043259000.Id55c74755f0a431ce31223acc86865718ae0c123@gnutoolchain-gerrit.osci.io>
` (2 preceding siblings ...)
2019-10-15 12:46 ` Simon Marchi (Code Review)
@ 2019-10-15 13:05 ` Andrew Burgess (Code Review)
2019-10-15 13:52 ` Tom de Vries (Code Review)
2019-10-15 16:02 ` Simon Marchi (Code Review)
5 siblings, 0 replies; 6+ messages in thread
From: Andrew Burgess (Code Review) @ 2019-10-15 13:05 UTC (permalink / raw)
To: Tom de Vries, gdb-patches; +Cc: Simon Marchi
Andrew Burgess has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................
Patch Set 1: Code-Review+1
The explanation sounds reasonable, the code looks good to me. I don't claim to be an x86-64 ABI expert though, but assuming no test regressions I'm happy with this change.
--
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings
^ permalink raw reply [flat|nested] 6+ messages in thread* Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64
[not found] <gerrit.1571043259000.Id55c74755f0a431ce31223acc86865718ae0c123@gnutoolchain-gerrit.osci.io>
` (3 preceding siblings ...)
2019-10-15 13:05 ` Andrew Burgess (Code Review)
@ 2019-10-15 13:52 ` Tom de Vries (Code Review)
2019-10-15 16:02 ` Simon Marchi (Code Review)
5 siblings, 0 replies; 6+ messages in thread
From: Tom de Vries (Code Review) @ 2019-10-15 13:52 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Simon Marchi
Tom de Vries has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................
Patch Set 1:
> Patch Set 1:
>
> (1 comment)
>
> So, it LGTM, but is there anybody else that you think should review it? Andrew or Alan perhaps?
Andrew already reviewed by now :)
Given that we're still not at +2, we could ask Alan, but he hasn't registered yet, so AFAIU I can't mark him as reviewer.
--
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings
^ permalink raw reply [flat|nested] 6+ messages in thread* Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64
[not found] <gerrit.1571043259000.Id55c74755f0a431ce31223acc86865718ae0c123@gnutoolchain-gerrit.osci.io>
` (4 preceding siblings ...)
2019-10-15 13:52 ` Tom de Vries (Code Review)
@ 2019-10-15 16:02 ` Simon Marchi (Code Review)
5 siblings, 0 replies; 6+ messages in thread
From: Simon Marchi (Code Review) @ 2019-10-15 16:02 UTC (permalink / raw)
To: Tom de Vries, gdb-patches; +Cc: Andrew Burgess
Simon Marchi has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................
Patch Set 1: Code-Review+2
> Patch Set 1:
>
> > Patch Set 1:
> >
> > (1 comment)
> >
> > So, it LGTM, but is there anybody else that you think should review it? Andrew or Alan perhaps?
>
> Andrew already reviewed by now :)
>
> Given that we're still not at +2, we could ask Alan, but he hasn't registered yet, so AFAIU I can't mark him as reviewer.
Yeah, well given the quite extensive test and the fact that Andrew also looked at it, I think it's pretty safe to merge. Alan, you are welcome to review this post-merge, of course :)
--
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings
^ permalink raw reply [flat|nested] 6+ messages in thread