padding is needed for 64-bit big endian
ClosedPublic
Actions

Authored by • gniibe on Apr 7 2017, 1:07 AM.

Details

Reviewers

justus
dkg

Commits

rGbf8b5e9042b3: gpgscm: Fix compact vector encoding.

Summary

We have an issue of gpgscm on s390x.
The _flag field should be compatible to vector element for gc.
On the machine with 64-bit big endian, LSB of _flag is not the LSB of the word of vector element when we don't have this fix.

This is not yet perfect patch, it's just for testing.
This doesn't work for LLP64 system (we need long long there), only for LP64 or LP32.

For real use, we need to detect endian-ness and size of int and add padding.

Test Plan

On porterbox, I'll test if it works.

Diff Detail

Repository

rG GnuPG

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

• gniibe created this revision.Apr 7 2017, 1:07 AM

• gniibe mentioned this in T3014: Intermittent crashes in gpgscm on s390x.

Update of the diff, so that we can keep _flag field access to 32-bit on 64-bit machine.

Please decide for _flag access on 64-bit machine, if 32-bit access is better or not.
If 64-bit access is better, update version of {Diff1255} is needed, instead.
unsigned long is not good for LLP64 system. Use unsigned long long for 64-bit system.

@gniibe thanks for figuring this out. This is rather mundane, I'm somewhat astonished that I did not think of that :(.

Do you know why this is only sometimes causing problems? I have never once seen a crash on my s390 emulator, yet @dkg seems to have no problem reproducing it.

Wouldn't a nicer and simpler fix be to use a machine-word-sized integer for _flag? Do you see any downside to that?

justus awarded a token.Apr 7 2017, 11:53 AM

I think that there are two archs: s390 and s390x. Latter is 64-bit and supports 32-bit version as well.
Use of machine word size (32-bit for 32-bit machine, 64-bit for 32-bit machine) is good. That will be update of Diff 1255.
But I don't know how it is achieved easily.
(If we can ignore LLP64, we can use unsigned long.)
Please go ahead that way.

In D421#2968, @gniibe wrote:

I think that there are two archs: s390 and s390x. Latter is 64-bit and supports 32-bit version as well.

I believe I have an emulated s390x:

$ uname -a
Linux s390 4.9.0-2-s390x #1 SMP Debian 4.9.13-1 (2017-02-27) s390x GNU/Linux

(gdb) print sizeof (void *)
$1 = 8
(gdb) print sizeof (int)
$2 = 4

Use of machine word size (32-bit for 32-bit machine, 64-bit for 32-bit machine) is good. That will be update of Diff 1255.
But I don't know how it is achieved easily.
(If we can ignore LLP64, we can use unsigned long.)

uintptr_t should do the trick.

I understand your emulator is s390x. Perhaps, on the emulator, memory layout is different?
I now see that C99 is OK for GnuPG, or at least no problem for gpgscm.