Double conversion of struct stat

[zeldin]

When responding to the syscall SYS_stat (or related variants), pk calls the function copy_stat() to convert the struct frontend_stat provided by spike into a regular struct stat. However, after returning from the syscall, newlib calls the function _conv_stat(), which also converts to a regular struct stat, this time from a struct kernel_stat (which looks identical to the struct frontend_stat). So the stat struct gets converted twice, with a garbage result.

I don't know whether it's the pk or newlib which should stop converting, I guess pk should use the same output format as the linux kernel does? (Edit: The riscv native linux kernel, I mean.)

[aswaterman]

Newlib programs that used struct stat previously worked, so this sounds like a newlib regression. I believe pk already matches the linux kernel in this regard, given that glibc programs once ran upon it (before they started needing more syscalls than we currently provide).

[zeldin]

Hm, it could be that glibc and newlib have different definitions of struct stat; the format of the data output by pk will depend on that definition, and thus also on which set of include files you compile pk against. Which seems wrong in itself, since the output of the pk should not be dependent on which libc the application is using.

The newlib code doesn't seem to have been changed since Dec 26, 2017...

[aswaterman]

Well, pk is supposed to adhere to the Linux ABI, so if glibc programs run correctly, then pk is probably correct.

[zeldin]

I tested just now. It does not work with a glibc program either.

I think it's like this:

• glibc uses a definition of struct stat which is identical to the kernel (/ frontend) stat. It does not convert the struct.
• newlib uses a different definition of struct stat, and performs a conversion from the kernel variant to its own.
• If you compile pk using glibc headers, the function copy_stat becomes a no-operation (or more precisely a plain memcpy). Then it works with both glibc and newlib programs.
• If you compile pk using newlib headers, the function copy_stat changes the format, and then it neither works with glibc or newlib, since they both expect the kernel format.

So based on this I believe the correct thing to do is to replace the call to copy_stat in pk with a call to memcpy. I have verified that this works with both glibc and newlib programs. And the behaviour of memcpy is not dependant on which set of include files you compile pk with.

I'll create a pull request with this change.

[aswaterman]

But that means that if you compile pk with newlib headers, it doesn't comply to the Linux ABI.

I think the right solution is to not depend on the headers and to conform to the Linux ABI.

[zeldin]

Yup. If you compile the current pk sources with newlib, it does not comply with the Linux ABI. If we change it to use memcpy instead of copy_stat, then it complies to the Linux ABI and does not depend on headers.

[aswaterman]

OK, cool.

[zeldin]

(Ostensibly, the frontend_stat struct could be different from the kernel format, and warrant a different copy_stat variant, but I don't think there is any benefit in doing that. The current frontend_stat looks specifically crafted to match the kernel one.)