[saagarjha]

Someone here probably knows this, so I figured I'd ask: I submitted a small patch to the kernel a while back and I made a typo in it and it broke the tests, and then I couldn't figure out how to get the test bot to pick up my new patch and by that point I assume anyone who would possibly have reviewed my patch stopped bothering. Is there something I should do to get it looked at again?

[stefan_]

If your original patch contained a mistake, you just send the new version with the v2 prefix to the mailing list. You can do this with git format-patch -v2 and then send-email will pick it up automatically.

You should note the change in the individual commit or the cover letter.

[saagarjha]

Hmm, I’ll take a look. I think I manually did some munging of the subject line to add “v2” where it seemed appropriate but I probably did it wrong.

[DSingularity]

E-mail the person maintaining the sub-system you are modifying.

[saagarjha]

I had Al Viro CC'd on the emails when I sent it to linux-fsdevel. Are you saying I should send it to him again, directly? (The patch, for reference: https://github.com/saagarjha/linux/commit/4867a403decc364c8b.... I figured I should also take this opportunity to check to make sure there's nothing wrong with it that might cause it to be ignored.)

[megous]

In what sense does it overflow?

    printf("%llx\n", ((long long)0xffffffff << 32) | 0xffffffff);

Prints:

    ffffffffffffffff

[kazinator]

The idea may be to avoid shifting a 1 into the sign bit, which is undefined behavior.

The values in question are being convertd back to loff_t anyway, but that's an implementation-defined conversion rather than undefined behavior.

So that is to say, doing the shift in the corresponding unsigned type and then converting to the original signed type is better defined at the language level than just doing the shift in the signed type.

I think this is just academic, because shifting a 1 into the sign bit behaves the same way on all platforms targeted by Linux using the only compiler that you can use.

Well, or maybe not. The one problem there is new changes in GCC which assume that all sorts of things that used to work (and were thus de facto implementation-defined) are no longer happening in programs (because they are undefined behavior, so who would do that?) Thus even if the target hardware has stable two's complement behavior on a left shift that alters the sign of a value, and that has worked fine for years, an updated GCC can suddenly decide that it's okay optimize based on the idea that the program doesn't do such a thing.

On a different note, the proposed change is jarringly ugly because u64 is size specific, whereas loff_t isn't. A long long isn't defined as 64 bits; it is at least 64 bits. On a system that, say, makes long long 128 bits, the proposed code change is not correct. To make this change with the proper elegance and precision, what you need is for loff_t to have a sister unsigned type that pairs with it: a uloff_t.

[saagarjha]

> Well, or maybe not. The one problem there is new changes in GCC which assume that all sorts of things that used to work (and were thus de facto implementation-defined) are no longer happening in programs (because they are undefined behavior, so who would do that?) Thus even if the target hardware has stable two's complement behavior on a left shift that alters the sign of a value, and that has worked fine for years, an updated GCC can suddenly decide that it's okay optimize based on the idea that the program doesn't do such a thing.

Note that Linux compiles with -fwrapv or one of the similar flags that defines overflow to wrap, so this isn’t an issue. I guess this trivializes this patch more, but I figured that they might want the patch anyways in case they ever move off of the flag. (I couldn’t find a uloff_t, so I copied what other code seemed to be doing in this case.)

[megous]

Thanks for your detailed response!

[saagarjha]

I’m not at my computer, so I can’t double-check this, but IIRC the issue was that ((long long)0xffffffff << 32) is undefined because it shifts past 2^63-1, which causes an overflow of the long long. (I know the undefined behavior sanitizer flagged some equivalent code in a Linux reimplementation I was working on, which is how I noticed it in the kernel.)

[crististm]

Yes, if you know him personally; no otherwise

[saagarjha]

I do not, unfortunately :(