fix Issue 16274 - short argument passed in 16 bit register #11651

WalterBright · 2020-08-30T10:04:17Z

…6-bit register, against ABI

dlang-bot · 2020-08-30T10:04:19Z

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	16274	major	The curses of debugging: short argument passed in 16-bit register, against ABI

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#11651"

wilzbach · 2020-08-30T10:09:42Z

Test?

UplinkCoder · 2020-08-30T15:33:42Z

@WalterBright I think 1byte values should be promoted to 4byte.
Rather than 2byte.

luismarques · 2020-08-30T15:48:19Z

src/dmd/e2ir.d

+            /* Do integral promotions. This is not necessary per the C ABI, but
+             * some code from the C world seems to rely on it.


This description is a bit misleading. Integral promotion is a language-level concept, specified by the language spec. The use of zero/sign-extension when passing arguments in registers is something specified by the platform-specific ABI (psABI). Of course, the two are closely related. Still, this mixes concepts that describe things at different levels, and which don't always map cleanly between each other. For instance, the RISC-V psABI does zero/sign-extension to 32 bits and then sign-extension to 64 bits (for RV64), which is something that would be surprising when reasoning in terms of integral promotions.

Anyway, I just quickly checked the latest version of the x86-64 psABI and it seems that it's still omissive about the issue addressed by this PR. That's quite surprising, as this is one of the fundamental things that a psABI specifies. The only relevant thing I found was a footnote about _Bool, saying that bits 7-1 are specified to be 0, but the others are unspecified. That is consistent with the general rule about the unused bits being unspecified. Microsoft's x86-64 ABI document is more explicit about this and says that "All integer arguments in registers are right-justified, so the callee can ignore the upper bits of the register and access only the portion of the register necessary". (Even that description is ambiguous, as it's still consistent with the upper bits being specified by some rule).

I will try to gather more information about how this is currently being handled in the real-world by various compilers. In any case, I suppose there's no major cost for this "fix", even if this has been addressed by more recent versions of the compilers and the curses library, so it probably makes sense to merge it. I just recommend rewording the comment.

Thank you for doing the research on this. Unfortunately, we can't tell people to fix their C code, they'll just say "D is broken". We simply have to generate code that will work with their C code, regardless of what the spec says.

Unfortunately, we can't tell people to fix their C code, they'll just say "D is broken". We simply have to generate code that will work with their C code, regardless of what the spec says.

I agree. I was just wondering if the situation on the ground could have changed in the meanwhile. The relevant compilers could have been updated and newer versions of the libraries distributed, such that this no longer was an actual issue for reasonably recent distributions. I'll delve into this later. It seems that, at least for a while, some major compilers were disagreeing about this issue.

I didn't have time to check more deeply but it seems that GCC is conservative both on the caller side (clearing out the upper bits before calling) and the callee side (zero/sign-extending before using the arguments), while Clang is conservative on the caller side but optimistically assumes that they were already extended when using them on the callee side. I tested with versions of GCC and Clang from trunk to quite old versions, and that remained the same. This is consistent with the bug report, which was reported for OS X, so the library was probably compiled with Clang. The curses library is incorrectly implementing a function with an int parameter that is declared as a short in the prototype. In theory that's undefined behavior, in practice it exposes these differences in how the ABI is handled by the various compilers. I agree that the sensible solution is for DMD to also be conservative on the caller side. On the callee side we're probably safe, given that Clang has lived with its approach for a long while.

The curses library is incorrectly implementing a function with an int parameter that is declared as a short in the prototype.

I can't see that in the code I posted, what I see is (after expanding macros)

int _nc_init_pair(SCREEN *sp, int pair, int f, int b) { // ... } int init_pair_sp (SCREEN *sp, short pair, short f, short b) { return _nc_init_pair(sp, pair, f, b); } int init_pair (short pair, short f, short b) { return init_pair_sp(CURRENT_SCREEN, pair, f, b); }

I can't see that in the code I posted, what I see is (after expanding macros)

Thanks. I was writing that bit from memory, and I probably also misdiagnosed that when I last looked at it. That further cements that we need this fix.

ibuclaw · 2020-08-30T15:50:26Z

src/dmd/e2ir.d

@@ -268,6 +268,43 @@ private elem *callfunc(const ref Loc loc,
                 */
                ea.Ety = TYllong;
            }
+
+            /* Do integral promotions. This is not necessary per the C ABI, but
+             * some code from the C world seems to rely on it.


Yes, it is necessary for the C ABI. On x86_64, all integral types smaller than int should actually be passed as an int. This apply to both parameters in the protoype and variadic parameters.

My reading of the ABI doesn't make this clear. 16274 is the only case I've ever heard of where it's a problem. I also tries various cases with gcc, and on the receiving end I can't get it to rely on the upper bits.

I was reading the compiler docs: https://gcc.gnu.org/onlinedocs/gccint/Stack-Arguments.html#TARGET_005fPROMOTE_005fPROTOTYPES

For x86_64 targets, this hook is set to true.

WalterBright · 2020-08-30T22:51:55Z

@UplinkCoder they're promote to int.

JohanEngelen · 2020-08-30T23:19:33Z

test/runnable/mars1.d

+{
+    // provide alternate implementation of .pair()
+    pragma(mangle, "pair")
+    extern(C) static void pair(int a, int b, int c, int d)


wouldn't it be much better to implement this on the C side, to test the C-ABI interop?
The point of this change is not to force arguments to become int, it's to match C ABI, which is slightly different (imagine a platform where the C ABI is not to promote to int).

and explicitly test this with some extreme values where things may go wrong (2 and 4 don't sound very difficult)

wouldn't it be much better to implement this on the C side, to test the C-ABI interop?

I couldn't get gcc to access those upper bits.

Tests are better self-contained as much as practical.

There are plenty of other tests done in C that check short and int arguments.

Any unpredictable C ABI changes are going to cause unpredictable problems, experience shows it's a waste of time attempting to predict them. The wackiest one is the Win64 one which took a left turn at Alberqueue into bizzaro land. Nobody could have predicted that.

Yeah, this test totally won't pass on ARM, SPARC, or POWER (to name three widely supported alternative targets).

I couldn't get gcc to access those upper bits.

The original implementation of init_pair is here:
https://github.com/mirror/ncurses/blob/790a85dbd4a81d5f5d8dd02a44d84f01512ef443/ncurses/base/lib_color.c#L556-L703

Though as the host platform in the bug report is OSX, I suspect it is clang that is compiling the C code.

Any unpredictable C ABI changes are going to cause unpredictable problems, experience shows it's a waste of time attempting to predict them.

Isn't that the point of testcases though? To detect when things no longer work and a fix is needed?

If D's extern (C) isn't working, not much is going to work. Here, we don't need to test that extern (C) is working.

src/dmd/e2ir.d

WalterBright · 2020-09-07T23:21:43Z

This is a good fix, and needs to get pulled.

luismarques · 2020-09-08T10:00:13Z

test/runnable/mars1.d

+	//printf("%d %d %d %d\n", a, b, c, d);
+	assert(a == -1);
+	assert(b == 2);
+	assert(c == -3);
+	assert(d == 4);


Bad indentation?

luismarques · 2020-09-08T10:30:33Z

I recommend the following changes to this PR, in decreasing order of importance:

Only run the code to extend the arguments and the test on X86 / x86_64. This is a mitigation for differences in how the x86 ABI is implemented in C compilers, so it doesn't make sense for it to apply more broadly, and I expect actual issues to arise for other platforms if the code and the test are run unconditionally.
Tweak the comment to better reflect that this applies to the x86 ABI, not some generic C ABI.
Fix the indentation issue.

The implementation of the test seems fine to me as is (other than running unconditionally)..

ibuclaw

Implement seems fine, but the test is not.

ibuclaw · 2020-09-08T11:10:46Z

Only run the code to extend the arguments and the test on X86 / x86_64. This is a mitigation for differences in how the x86 ABI is implemented in C compilers, so it doesn't make sense for it to apply more broadly, and I expect actual issues to arise for other platforms if the code and the test are run unconditionally.

Doing a quick grep for the referenced ABI hook that controls this promoting behaviour, these are the targets that look like the test will pass on:

i386      // Primary
x86_64    // Primary

Whereas these are targets that the test will fail on

aarch64   // Primary
arm       // Primary
mips      // Primary
powerpc   // Primary
systemz   // Secondary
sparc     // Primary

Where the target is a primary/secondary supported platform, I've made it clear as comments.

(I could post the full CPU list - there are 55 in total - but this'll just add noise to the current review).

WalterBright · 2020-09-09T03:03:39Z

Whereas these are targets that the test will fail on

Since this passes all of the tests in the test suite, I suggest not worrying about it. When the tests are run on a platform where it won't work, then the test can be adjusted. There are likely quite a lot of x86 specific tests in the suite.

For people who need an explanation of why that test is in the test suite, that's why there's a link in it to the Bugzilla issue which contains a link here. I often make use of this when researching why a piece of code exists.

ibuclaw · 2020-09-09T10:08:45Z

No, I do worry about it, because people test D on the platforms I listed very frequently. For example, I already know that I broke Solaris/SPARC from a patch I committed yesterday. The DMD testsuite passing is neither representative of the genuine concerns raised, nor the broader platforms that D is being ran on.

I've ran a modified version of the test on a few servers.

extern(C) int pair(short a, ushort b, byte c, ubyte d);
extern(C) int printf(const(char) *fmt, ...);

struct S
{
    // provide alternate implementation of .pair()
    pragma(mangle, "pair")
    extern(C) static void pair(int a, int b, int c, int d)
    {
        printf("%d %d %d %d\n", a, b, c, d);
        assert(a == -1);
        assert(b == 2);
        assert(c == -3);
        assert(d == 4);
    }
}

int public_pair(short a, ushort b, byte c, ubyte d)
{
    return pair(a, b, c, d);
}

void main()
{
    public_pair(-1, 2, -3, 4);
}

On AArch64:

65535 2 253 4
core.exception.AssertError@test.d(11): Assertion failure

On PPC64LE

-1 2 -3 4

On SPARC

-1 2 -3 4

On x86_64

65535 1919352834 1919354877 4
core.exception.AssertError@test.d(11): Assertion failure

luismarques · 2020-09-09T11:53:35Z

On x86_64

65535 1919352834 1919354877 4
core.exception.AssertError@test.d(11): Assertion failure

Based on this result I assume that you ran the test with the existing (non-extending) behaviour. I guess an interesting question is what the results are when you also apply the patch.

ibuclaw · 2020-09-09T14:35:38Z

Based on this result I assume that you ran the test with the existing (non-extending) behaviour. I guess an interesting question is what the results are when you also apply the patch.

The patch can't be applied, because it is specific to DMD.

https://d.godbolt.org/z/zjf7nM

ibuclaw · 2020-09-10T08:46:17Z

FYI @WalterBright you've already fixed at least half the issue in #11648. So using integer constants won't trigger the possible problem anymore.

ibuclaw · 2020-09-10T10:26:42Z

Alternative fix made in #11719. Which does promotions in the front-end using existing machinery and a new target hook so the compiler back-end can decide what it wants to do.

WalterBright · 2020-09-11T08:02:56Z

dmd's backend is x86, x86-64 only so it never needs to decide what to do.

WalterBright · 2020-11-08T06:27:30Z

Implement seems fine, but the test is not.

@ibuclaw when requesting changes, please be much more specific.

ibuclaw · 2020-11-08T14:32:55Z

Implement seems fine, but the test is not.

@ibuclaw when requesting changes, please be much more specific.

I've already proved that the test doesn't work on ARM64.
This isn't an x86 kludge of the ABI, but something explicitly done on a number of targets. https://gcc.gnu.org/onlinedocs/gccint/Stack-Arguments.html#TARGET_005fPROMOTE_005fPROTOTYPES
We already have integer promotion being handled by the frontend for variadic parameters, promoting prototypes in the frontend fixes the issue for all D compilers that require it for their target.

WalterBright · 2020-11-09T07:21:43Z

I've already proved that the test doesn't work on ARM64.

I versioned it so the test is only for X86 and X86_64.

This isn't an x86 kludge of the ABI, but something explicitly done on a number of targets. https://gcc.gnu.org/onlinedocs/gccint/Stack-Arguments.html#TARGET_005fPROMOTE_005fPROTOTYPES

It's clearly a codegen issue, not a front end one.

We already have integer promotion being handled by the frontend for variadic parameters, promoting prototypes in the frontend fixes the issue for all D compilers that require it for their target.

Variadic integral promotion is required by the language spec, which is entirely different from this promotion which is an ABI thing. Doing it in the front end means changing the front end's notion of the type, which is wrong and will cause problems, such as with D's inliner. The inliner runs after the semantic pass and before the code generator, and presumes the argument types are all consistent with the parameter types. Making them inconsistent will likely cause problems. I agree the inliner really belongs in the backend, but that is a project for some other time.

If you need to do this a different way for your supported targets, feel free, but this PR is the right way for DMD.

ibuclaw · 2020-11-09T10:27:32Z

Doing it in the front end means changing the front end's notion of the type, which is wrong and will cause problems, such as with D's inliner. The inliner runs after the semantic pass and before the code generator, and presumes the argument types are all consistent with the parameter types.

The type doesn't change - the parameter in short fun(short s) remains a 16-bit value, it's just the way that it's passed is fun(cast(int) short_value). Wouldn't such casting be stripped by dmd's inliner?

If you need to do this a different way for your supported targets, feel free, but this PR is the right way for DMD.

It's more a violation of the DRY principle. This is just repeating what integralPromotions already does, and in a way that only benefits DMD.

WalterBright · 2020-11-09T10:33:38Z

The argument type passed to the function call no longer matches the parameter type. This mismatch will cause problems now or later. I really do not want to mess with this kind of hack, given the number of people that work on the compiler.

The frontend and the backend do repeat themselves here and there, but that's necessary to keep them distinct code bases. It's better to keep it that way. Besides, it's not like the definition of C integer promotions is going to change.

ibuclaw · 2020-11-14T08:05:04Z

@kinke - would it fit better in ldc if prototypes were promoted by integralPromotions, or if you duplicated its logic in the code generator?

kinke · 2020-11-14T14:35:25Z

It's IMO better to keep things as they are; we handle it like clang and use a LLVM parameter attribute to have LLVM sign- or zero-extend the value.

RazvanN7 · 2021-01-08T12:10:09Z

@ibuclaw Given the above discussion and @kinke 's reply, is it ok if we merge this and close #11719 ? @WalterBright can you please rebase so that everything is ready in case @ibuclaw accepts this?

dlang-bot added the Bug Fix label Aug 30, 2020

luismarques reviewed Aug 30, 2020

View reviewed changes

ibuclaw reviewed Aug 30, 2020

View reviewed changes

WalterBright force-pushed the fix16274 branch 4 times, most recently from 277ac02 to b7a1e24 Compare August 30, 2020 21:41

WalterBright changed the title ~~fix Issue 16274 - The curses of debugging: short argument passed in 1…~~ fix Issue 16274 - short argument passed in 1… Aug 30, 2020

WalterBright changed the title ~~fix Issue 16274 - short argument passed in 1…~~ fix Issue 16274 - short argument passed in 16 bit register Aug 30, 2020

JohanEngelen reviewed Aug 30, 2020

View reviewed changes

WalterBright force-pushed the fix16274 branch 2 times, most recently from ba46843 to e74987d Compare September 4, 2020 09:30

ibuclaw reviewed Sep 4, 2020

View reviewed changes

src/dmd/e2ir.d Outdated Show resolved Hide resolved

ibuclaw reviewed Sep 4, 2020

View reviewed changes

src/dmd/e2ir.d Outdated Show resolved Hide resolved

luismarques reviewed Sep 8, 2020

View reviewed changes

ibuclaw requested changes Sep 8, 2020

View reviewed changes

WalterBright force-pushed the fix16274 branch from e74987d to 9ebb54c Compare September 9, 2020 03:06

ibuclaw mentioned this pull request Sep 10, 2020

fix Issue 16274 - short argument passed in 16 bit register #11719

Closed

WalterBright force-pushed the fix16274 branch from 9ebb54c to e66eb8c Compare November 8, 2020 06:28

WalterBright force-pushed the fix16274 branch from e66eb8c to bc2f238 Compare November 9, 2020 07:09

RazvanN7 added the Needs Rebase label Jan 8, 2021

WalterBright force-pushed the fix16274 branch from bc2f238 to 8b96de9 Compare January 23, 2021 08:06

dlang-bot removed the Needs Rebase label Jan 23, 2021

dlang-bot added stalled Needs Rebase labels May 20, 2021

fix Issue 16274 - short argument passed in 16-bit register, against ABI

0027121

WalterBright force-pushed the fix16274 branch from 8b96de9 to 0027121 Compare May 30, 2021 08:46

dlang-bot removed Needs Rebase stalled labels May 30, 2021

ibuclaw approved these changes May 30, 2021

View reviewed changes

ibuclaw added the auto-merge label May 30, 2021

dlang-bot merged commit 3aab699 into dlang:master May 30, 2021

		/* Do integral promotions. This is not necessary per the C ABI, but
		* some code from the C world seems to rely on it.

fix Issue 16274 - short argument passed in 16 bit register #11651

fix Issue 16274 - short argument passed in 16 bit register #11651

Conversation

WalterBright commented Aug 30, 2020

dlang-bot commented Aug 30, 2020 • edited

Bugzilla references

Testing this PR locally

wilzbach commented Aug 30, 2020

UplinkCoder commented Aug 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luismarques Sep 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterBright commented Aug 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuclaw Aug 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterBright commented Sep 7, 2020

Choose a reason for hiding this comment

luismarques commented Sep 8, 2020

ibuclaw left a comment

Choose a reason for hiding this comment

ibuclaw commented Sep 8, 2020 • edited

WalterBright commented Sep 9, 2020

ibuclaw commented Sep 9, 2020

luismarques commented Sep 9, 2020

ibuclaw commented Sep 9, 2020

ibuclaw commented Sep 10, 2020

ibuclaw commented Sep 10, 2020

WalterBright commented Sep 11, 2020

WalterBright commented Nov 8, 2020

ibuclaw commented Nov 8, 2020 • edited

WalterBright commented Nov 9, 2020

ibuclaw commented Nov 9, 2020

WalterBright commented Nov 9, 2020

ibuclaw commented Nov 14, 2020

kinke commented Nov 14, 2020

RazvanN7 commented Jan 8, 2021 • edited

dlang-bot commented Aug 30, 2020 •

edited

luismarques Sep 8, 2020 •

edited

ibuclaw Aug 31, 2020 •

edited

ibuclaw commented Sep 8, 2020 •

edited

ibuclaw commented Nov 8, 2020 •

edited

RazvanN7 commented Jan 8, 2021 •

edited