Internal BitRotateL/R functions (and their aliases) return incorrect results when first argument is negative.

[osir3z]

Examples

BitRotateL($80000000, 4) returns -8, i.e. $FFFFFFF8, but it should return $00000008.
BitRotateR($80000000, 4) returns -134217728, i.e. $F8000000, but it should return $08000000.

Cause

Bit rotation is simulated by OR-ing together a left and a right bit shift.

AviSynthPlus/avs_core/core/parser/script.cpp

Lines 697 to 707 in c377916

	static int a_rol(int value, int shift) {
	if ((shift &= sizeof(value)*8 - 1) == 0)
	return value;
	return (value << shift) | (value >> (sizeof(value)*8 - shift));
	}
	static int a_ror(int value, int shift) {
	if ((shift &= sizeof(value)*8 - 1) == 0)
	return value;
	return (value >> shift) | (value << (sizeof(value)*8 - shift));
	}

But since the inputs are signed the right shifts are treated as arithmetic and are extending the sign bit.

Solution

The right shifts, on lines 700 and 706 above, just need to be recast similarly to the BitRShiftL function.

AviSynthPlus/avs_core/core/parser/script.cpp

Line 694 in c377916

AVSValue BitRShiftL(AVSValue args, void*, IScriptEnvironment* ) { return int(unsigned(args[0].AsInt()) >> unsigned(args[1].AsInt())); }

[pinterf]

Good catch, in Avisynth 2.6 a_ror and a_rol was implemented only in assembly code, see below.
Probably when Avisynth+ project replaced asm code with C, they failed with this conversion.

Old one:

int __declspec(naked) __stdcall a_rol(int arg1, int arg2) { // asm rol r/m32, CL
    __asm {
        mov  eax, [esp+4]
        mov  ecx, [esp+8]
        rol  eax, cl
        ret  8
    }
}

[osir3z]

Yeah, I noticed that while I was checking to see how long the current code has been live. Turns out it's been 10 years. I suppose that just shows how often people use bit rotation in their scripts.

Honestly, I was debugging my script for a few hours before I bothered to check the result of the rotation. And even once I knew it was wrong, I kept wondering if it was me that was wrong.

Anyway, I went down the rabbit hole and found the offending code, but I also looked at the definitions for the C++ bit shift operators. It turns out that, prior to the C++20 specification, right shifts on negative lhs is implementation defined (see https://en.cppreference.com/w/cpp/language/operator_arithmetic). Most implementations perform arithmetic shifts, but not all.

My point in mentioning this is that if you really want to be strict and ensure cross-compiler/platform compatibility, it might be worth casting all lhs' in the shift/rotate operations as unsigned, and sanitizing the rhs. Just a thought.

Keep up the excellent work. You folks are amazing.