Fix Issue 21158 - DWARF: function arguments are represented in reverse order

[Luhrel]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @Luhrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	21158	trivial	DWARF: function arguments are represented in reverse order

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#11630"

[kinke]

The argument reversal (ABI-wise) is restricted to extern(D) (and no , ... variadics), and that looks not to be accounted for. The this/context pointer is always the first param, not reversed.

[Luhrel]

The argument reversal (ABI-wise) is restricted to extern(D) (and no , ... variadics), and that looks not to be accounted for. The this/context pointer is always the first param, not reversed.

I think I get it done in the right way now.

[Geod24]

The commit title needs to be Fix [Issue] XXX, the PR title doesn't matter to the bot (because changelog is generated from git log), which is why you can see a little cross in the "Auto-Close" column of the bot's message.

[Luhrel]

The commit title needs to be Fix [Issue] XXX, the PR title doesn't matter to the bot (because changelog is generated from git log), which is why you can see a little cross in the "Auto-Close" column of the bot's message.

Good to know.

[kinke]

I think I get it done in the right way now.

Have you checked the this pointer for methods? Another implicit, non-formal param is the sret pointer in case the return value is allocated and its address passed by the caller (e.g., for non-POD return types such as a struct with dtor). - I've seen that you compare the DMD DWARF emission to LDC's; I think the better reference is probably GDC, as Iain has put quite some effort into a nice debugging experience AFAIK.

[12345swordy]

Looks good to me. Any objections @WalterBright ?

[RazvanN7]

@kinke @WalterBright Any objections to this addition?

[kinke]

I'm still looking for an sret test, preferrably including a combined sret+this testcase, and a test for a nested function receiving an implicit context pointer.

[RazvanN7]

@Luhrel please give me a ping when you have added the test required by @kinke . Thanks!

[Luhrel]

ping @RazvanN7.

[RazvanN7]

It seems that the nested function test is still missing.

[Luhrel]

@RazvanN7 Should be good now.

[kinke]

Still no sret tests.

[kinke]

Something like:

struct S
{
    int x;
    ~this() {}
    S foo(int a, float b)
    {
        // breakpoint here; expected low-level params order: `<sret pointer>, <this pointer>, b, a`
        return S(x + a);
    }
}

[Luhrel]

Something like:

struct S
{
    int x;
    ~this() {}
    S foo(int a, float b)
    {
        // breakpoint here; expected low-level params order: `<sret pointer>, <this pointer>, b, a`
        return S(x + a);
    }
}

Oh ok. Now I have no idea how to check if a Symbol is a sret pointer. The backend isn't as clear as the frontend, unfortunately.
Any help is welcome.

[kinke]

You're probably looking for this:

dmd/src/dmd/backend/cc.d

Line 749 in 71d410d

F3hiddenPtr = 0x40000, // function has hidden pointer to return value

(should be in sfunc.Sfunc.Fflags3 too).

[Luhrel]

That flag isn't set in this test case (but the sret argument is there, so I'm a bit confused) 🤔

[RazvanN7]

It seems that this is the only place where F3hiddenPtr is set. Maybe you can check if @kinke 's code reaches that path. If yes, the only place where F3hiddenPtr is checked is here.

Maybe it does get set but it somehow gets zeroed along the way. Otherwise, there might be another param or combination of params that tells us if a function requires a sret or not.

Hope this helps.

[Luhrel]

It seems that this is the only place where F3hiddenPtr is set. Maybe you can check if @kinke 's code reaches that path. If yes, the only place where F3hiddenPtr is checked is here.

Maybe it does get set but it somehow gets zeroed along the way. Otherwise, there might be another param or combination of params that tells us if a function requires a sret or not.

Hope this helps.

I dug a little bit. F3hiddenPtr is only set for classes.
toObjFile . visit(ClassDeclaration) -> genTypeInfoForClass -> emitVtlb (1/2) -> toThunkSymbol -> Fflags3 |= F3hiddenPtr.

In toObjFile.visit(StructDeclaration), there's nothing about F3hiddenPtr/ Fflags3, so I may be missing here. However, I have a very poor understanding of the backend, so I have no idea what are the consequences of adding it.

Any ideas @kinke ?

[kinke]

I have no idea about DMD's backend either; after a quick glance, it looks like setting the flag (f.Fflags3 |= F3hiddenPtr) might be missing here:

dmd/src/dmd/glue.d

Lines 743 to 765 in 218d999

	if (retmethod == RET.stack)
	{
	// If function returns a struct, put a pointer to that
	// as the first argument
	.type *thidden = Type_toCtype(tf.next.pointerTo());
	char[5 + 10 + 1] hiddenparam = void;
	__gshared uint hiddenparami; // how many we've generated so far
	const(char)* name;
	if (fd.nrvo_can && fd.nrvo_var)
	name = fd.nrvo_var.ident.toChars();
	else
	{
	sprintf(hiddenparam.ptr, "__HID%u", ++hiddenparami);
	name = hiddenparam.ptr;
	}
	shidden = symbol_name(name, SCparameter, thidden);
	shidden.Sflags |= SFLtrue | SFLfree;
	if (fd.nrvo_can && fd.nrvo_var && fd.nrvo_var.nestedrefs.dim)
	type_setcv(&shidden.Stype, shidden.Stype.Tty | mTYvolatile);
	irs.shidden = shidden;
	fd.shidden = shidden;
	}