[PATCH] jscript: Force base type of _jsval_t to be ULONGLONG instead of double.
I guess that I should send the proposed fix to wine-devel instead of waiting for response in the bug report. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50845 Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/jscript/jsval.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dlls/jscript/jsval.h b/dlls/jscript/jsval.h index 963f59f6a08..d930d35f4e4 100644 --- a/dlls/jscript/jsval.h +++ b/dlls/jscript/jsval.h @@ -54,6 +54,7 @@ typedef enum { struct _jsval_t { #ifdef JSVAL_DOUBLE_LAYOUT_PTR32 union { + ULONGLONG ull; double n; struct { union { @@ -69,9 +70,10 @@ struct _jsval_t { #else jsval_type_t type; union { + ULONGLONG ull; + double n; IDispatch *obj; jsstr_t *str; - double n; BOOL b; VARIANT *v; } u; -- 2.30.2
Looks like a bug in Clang to me, although working around it is fine, but it's not my call. Nevertheless I think you should report it to Clang so they fix it.
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
Looks like a bug in Clang to me, although working around it is fine, but it's not my call.
Nevertheless I think you should report it to Clang so they fix it.
Why do you think that it's a clang bug? Using fldl + fstpl to store and fetch the double to/from the floating point stack is perfectly legitimate. -- Dmitry.
On 24/03/2021 15:02, Dmitry Timoshkov wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
Looks like a bug in Clang to me, although working around it is fine, but it's not my call.
Nevertheless I think you should report it to Clang so they fix it.
Why do you think that it's a clang bug? Using fldl + fstpl to store and fetch the double to/from the floating point stack is perfectly legitimate.
Well, because jsval_t is a union, not a double. Shouldn't assignment be a bitwise copy? The compiler can't know what actual type is used by the union here. Unless I misremember the C standard.
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
On 24/03/2021 15:02, Dmitry Timoshkov wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
Looks like a bug in Clang to me, although working around it is fine, but it's not my call.
Nevertheless I think you should report it to Clang so they fix it.
Why do you think that it's a clang bug? Using fldl + fstpl to store and fetch the double to/from the floating point stack is perfectly legitimate.
Well, because jsval_t is a union, not a double. Shouldn't assignment be a bitwise copy? The compiler can't know what actual type is used by the union here. Unless I misremember the C standard.
If I understand correctly, the compiler have chosen the memeber of a maximal bits width and used it for a copy. That's unfortunate that the chosen member happened to be of type 'double', and as a result floating point instructions were used to perform the copying operation. -- Dmitry.
On 24.03.2021 18:32, Dmitry Timoshkov wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
On 24/03/2021 15:02, Dmitry Timoshkov wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
Looks like a bug in Clang to me, although working around it is fine, but it's not my call.
Nevertheless I think you should report it to Clang so they fix it. Why do you think that it's a clang bug? Using fldl + fstpl to store and fetch the double to/from the floating point stack is perfectly legitimate.
Well, because jsval_t is a union, not a double. Shouldn't assignment be a bitwise copy? The compiler can't know what actual type is used by the union here. Unless I misremember the C standard. If I understand correctly, the compiler have chosen the memeber of a maximal bits width and used it for a copy. That's unfortunate that the chosen member happened to be of type 'double', and as a result floating point instructions were used to perform the copying operation.
Compiler is free to use any copy implementation it wants, but it can't affect stored values. If it changes stored values, it's obviously a compiler bug. But it would be good to understand what's exactly going on before jumping to that conclusion. From your description, I don't yet see the exact problem. I also can't reproduce it with my builds. Could you please send me your jscript.dll build? As I mentioned, when enums are involved, I'd suspect them because they are a known incompatibility between compilers. In 32-bit case, I don't see how they could cause a problem, but it may be worth trying to change them as well, like the attached patch. Jacek
Hi Jacek, Jacek Caban <jacek(a)codeweavers.com> wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
Looks like a bug in Clang to me, although working around it is fine, but it's not my call.
Nevertheless I think you should report it to Clang so they fix it. Why do you think that it's a clang bug? Using fldl + fstpl to store and fetch the double to/from the floating point stack is perfectly legitimate.
Well, because jsval_t is a union, not a double. Shouldn't assignment be a bitwise copy? The compiler can't know what actual type is used by the union here. Unless I misremember the C standard. If I understand correctly, the compiler have chosen the memeber of a maximal bits width and used it for a copy. That's unfortunate that the chosen member happened to be of type 'double', and as a result floating point instructions were used to perform the copying operation.
Compiler is free to use any copy implementation it wants, but it can't affect stored values. If it changes stored values, it's obviously a compiler bug. But it would be good to understand what's exactly going on before jumping to that conclusion. From your description, I don't yet see the exact problem. I also can't reproduce it with my builds. Could you please send me your jscript.dll build?
Did you find anything useful in jscript.dll that I sent you? -- Dmitry.
Hi Dmitry, On 4/2/21 8:19 AM, Dmitry Timoshkov wrote:
Hi Jacek,
Jacek Caban <jacek(a)codeweavers.com> wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
Looks like a bug in Clang to me, although working around it is fine, but it's not my call.
Nevertheless I think you should report it to Clang so they fix it. Why do you think that it's a clang bug? Using fldl + fstpl to store and fetch the double to/from the floating point stack is perfectly legitimate.
Well, because jsval_t is a union, not a double. Shouldn't assignment be a bitwise copy? The compiler can't know what actual type is used by the union here. Unless I misremember the C standard. If I understand correctly, the compiler have chosen the memeber of a maximal bits width and used it for a copy. That's unfortunate that the chosen member happened to be of type 'double', and as a result floating point instructions were used to perform the copying operation.
Compiler is free to use any copy implementation it wants, but it can't affect stored values. If it changes stored values, it's obviously a compiler bug. But it would be good to understand what's exactly going on before jumping to that conclusion. From your description, I don't yet see the exact problem. I also can't reproduce it with my builds. Could you please send me your jscript.dll build?
Did you find anything useful in jscript.dll that I sent you?
The good news is that I managed to reproduce it locally, so I could experiment with it. I'm leaning towards a solution like (a cleaner version of) the attached patch. It fixes the problem for me. I'm not yet sure about that, it needs another look, but it would be interesting to confirm that it works for you. Jacek
Jacek Caban <jacek(a)codeweavers.com> wrote:
Hi Dmitry,
On 4/2/21 8:19 AM, Dmitry Timoshkov wrote:
Hi Jacek,
Jacek Caban <jacek(a)codeweavers.com> wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
> Looks like a bug in Clang to me, although working around it is fine, but > it's not my call. > > Nevertheless I think you should report it to Clang so they fix it. Why do you think that it's a clang bug? Using fldl + fstpl to store and fetch the double to/from the floating point stack is perfectly legitimate.
Well, because jsval_t is a union, not a double. Shouldn't assignment be a bitwise copy? The compiler can't know what actual type is used by the union here. Unless I misremember the C standard. If I understand correctly, the compiler have chosen the memeber of a maximal bits width and used it for a copy. That's unfortunate that the chosen member happened to be of type 'double', and as a result floating point instructions were used to perform the copying operation.
Compiler is free to use any copy implementation it wants, but it can't affect stored values. If it changes stored values, it's obviously a compiler bug. But it would be good to understand what's exactly going on before jumping to that conclusion. From your description, I don't yet see the exact problem. I also can't reproduce it with my builds. Could you please send me your jscript.dll build?
Did you find anything useful in jscript.dll that I sent you?
The good news is that I managed to reproduce it locally, so I could experiment with it. I'm leaning towards a solution like (a cleaner version of) the attached patch. It fixes the problem for me. I'm not yet sure about that, it needs another look, but it would be interesting to confirm that it works for you.
The patch works. Thanks! -- Dmitry.
On 24.03.2021 12:33, Dmitry Timoshkov wrote:
I guess that I should send the proposed fix to wine-devel instead of waiting for response in the bug report.
Bug report is fine. I didn't know the answer without looking at this myself and I didn't have the time to do that earlier, but it was on my radar.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=50845 Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/jscript/jsval.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/dlls/jscript/jsval.h b/dlls/jscript/jsval.h index 963f59f6a08..d930d35f4e4 100644 --- a/dlls/jscript/jsval.h +++ b/dlls/jscript/jsval.h @@ -54,6 +54,7 @@ typedef enum { struct _jsval_t { #ifdef JSVAL_DOUBLE_LAYOUT_PTR32 union { + ULONGLONG ull; double n; struct { union {
Did you see a crash in 32-bit build as well?
@@ -69,9 +70,10 @@ struct _jsval_t { #else jsval_type_t type; union { + ULONGLONG ull; + double n; IDispatch *obj; jsstr_t *str; - double n; BOOL b; VARIANT *v; } u;
I think that there is a difference in enum underlying type between targets, which could explain the difference here. The attached patch works for me, please give it a try. Thanks, Jacek
Jacek Caban <jacek(a)codeweavers.com> wrote:
diff --git a/dlls/jscript/jsval.h b/dlls/jscript/jsval.h index 963f59f6a08..d930d35f4e4 100644 --- a/dlls/jscript/jsval.h +++ b/dlls/jscript/jsval.h @@ -54,6 +54,7 @@ typedef enum { struct _jsval_t { #ifdef JSVAL_DOUBLE_LAYOUT_PTR32 union { + ULONGLONG ull; double n; struct { union {
Did you see a crash in 32-bit build as well?
Sorry if I wasn't clear enough, the crash caused by assert(0) happens in 32-bit build, so your patch below won't help. 64-bit build is affected by another bug, and a diferent crash.
@@ -69,9 +70,10 @@ struct _jsval_t { #else jsval_type_t type; union { + ULONGLONG ull; + double n; IDispatch *obj; jsstr_t *str; - double n; BOOL b; VARIANT *v; } u;
I think that there is a difference in enum underlying type between targets, which could explain the difference here. The attached patch works for me, please give it a try.
-- Dmitry.
participants (3)
-
Dmitry Timoshkov -
Gabriel Ivăncescu -
Jacek Caban