Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[win32][arm64] Support Dark Theme on newer Windows builds #1172

Merged

Conversation

hmartinez82
Copy link
Contributor

@hmartinez82 hmartinez82 commented Apr 13, 2024

Continues #1048 #1045

Here's the disassembly:

00007FFA2A1D3E10 D503237F             pacibsp  
00007FFA2A1D3E14 F81F0FF3             str         x19,[sp,#-0x10]!  
00007FFA2A1D3E18 A9BF7BFD             stp         fp,lr,[sp,#-0x10]!  
00007FFA2A1D3E1C 910003FD             mov         fp,sp  
00007FFA2A1D3E20 B00007E8             adrp        x8,wil::details::g_enabledStateManager+40h (07FFA2A2D0000h)  
00007FFA2A1D3E24 B94BB913             ldr         w19,[x8,#0xBB8]  
00007FFA2A1D3E28 2A0003E1             mov         w1,w0  
00007FFA2A1D3E2C 912EE100             add         x0,x8,#0xBB8  
00007FFA2A1D3E30 97FF7910             bl          _InterlockedExchange (07FFA2A1B2270h)  
00007FFA2A1D3E34 2A1303E0             mov         w0,w19  
00007FFA2A1D3E38 A8C17BFD             ldp         fp,lr,[sp],#0x10  
00007FFA2A1D3E3C F84107F3             ldr         x19,[sp],#0x10  
00007FFA2A1D3E40 D50323FF             autibsp  
00007FFA2A1D3E44 D65F03C0             ret

The add x0,x8, instruction changed its parameter. So now the adrp x8,... instruction looks more stable because it uses a global variable (Using a global variable is also what is done when validating for the Intel x64 uxtheme.dll)

See https://developer.arm.com/documentation/ddi0602/2024-03/Base-Instructions/ADRP--Form-PC-relative-address-to-4KB-page-

  • functionPtr[0x10] & 0x1F) == 0x08 checks that the destination register is x8
  • (functionPtr[0x13] & 0x90) == 0x90 checks that the opcode is ADRP

Copy link
Contributor

github-actions bot commented Apr 13, 2024

Test Results

   418 files  ±0     418 suites  ±0   7m 19s ⏱️ -50s
 4 121 tests ±0   4 113 ✅ ±0   8 💤 ±0  0 ❌ ±0 
16 313 runs  ±0  16 221 ✅ ±0  92 💤 ±0  0 ❌ ±0 

Results for commit a0c4d79. ± Comparison against base commit b435cb8.

♻️ This comment has been updated with latest results.

@chirontt
Copy link
Contributor

Thanks for your quick work! I now have the dark scrollbars again in my Arm64 box. Here are how I test it:

  • merge this PR to my fork, and then locally rebuild the SWT native binaries by:
cd binaries\org.eclipse.swt.win32.win32.aarch64
del swt*.dll
mvn clean process-resources -Dnative=win32.win32.aarch64

The above mvn command would produce the updated SWT native (swt*.dll) files in the current directory.

  • create the module jar file which would embed the above SWT natives:
mvn clean verify

and the org.eclipse.swt.win32.win32.aarch64-3.126.0-SNAPSHOT.jar file is generated in the target directory.

  • manually run the Java test file Bug562043_DarkTableNoHover.java, using the above jar file in the classpath:
java -cp target\org.eclipse.swt.win32.win32.aarch64-3.126.0-SNAPSHOT.jar ^
..\..\tests\org.eclipse.swt.tests.win32\ManualTests\org\eclipse\swt\tests\win32\snippets\Bug562043_DarkTableNoHover.java

and the test program shows correct dark scrollbars in its window:

image

For comparison, here's the same window (with lighted scrollbars) before this fix:

image

@chirontt
Copy link
Contributor

Further more, I've built the complete Eclipse SDK with this fix, from my aggregator fork, and here's its main window showing nicely dark scrollbars on my Arm64 box:

image

In previous builds before this fix, it was showing lighted scrollbars, which alerted me to the changed uxtheme.dll file due to recent Windows Arm64 update.

@HannesWell
Copy link
Member

@niraj-modi or @SyntevoAlex could you please review this? I have no clue about the native code and no way to test it.

@SyntevoAlex
Copy link
Member

The new test looks quite weak to me, because adrp x8, ??? is a rather common instruction which doesn't really identify the function we need for the dark theme.

I suggest that instead both offsets 0xBB8 and 0xBD8 are recognized (to recognize both windows versions)

@hmartinez82
Copy link
Contributor Author

@SyntevoAlex ah good idea. I'll work on that tomorrow :)

@hmartinez82
Copy link
Contributor Author

@chirontt can you try with the latest changes? I changed the title of the PR too

@hmartinez82 hmartinez82 changed the title Validate SetPreferredAppMode() in ARM64 with ADRP instruction instead Accept two possible arguments for the ADD opcode on ARM64 validation of SetPreferredAppMode() May 10, 2024
@SyntevoAlex
Copy link
Member

It would be nice to also have code comments with Windows versions, like here:

/* Win10 builds from 21301 */
if ((functionPtr[0x31] == 0xBA) && // mov edx,
(*(const DWORD*)(functionPtr + 0x32) == 0xA91E)) // 0A91Eh
{
return TRUE;
}
/* Win11 builds from 22621 */
if ((functionPtr[0x15] == 0xBA) && // mov edx,
(*(const DWORD*)(functionPtr + 0x16) == 0xA91E)) // 0A91Eh
{
return TRUE;
}
return FALSE;

@SyntevoAlex
Copy link
Member

^ this is just a suggestion, I will still approve without it.

@hmartinez82
Copy link
Contributor Author

I don't have the old versions. @chirontt Can you find me the Windows builds before and after the original PR stopped working?

@SyntevoAlex SyntevoAlex changed the title Accept two possible arguments for the ADD opcode on ARM64 validation of SetPreferredAppMode() [win32][arm64] Support Dark Theme on newer Windows builds May 10, 2024
@chirontt
Copy link
Contributor

Can you find me the Windows builds before and after the original PR stopped working?

I'm not quite sure I follow you here. It's not the Eclipse build that stops working; it's the recent (last month's) Windows Arm64 update that causes the problem due to changes in the uxtheme.dll in the Windows update. With your latest Windows version running in your Arm64 box, the lighted scrollbar problem appears, for any Eclipse WoA versions.

You can grab the latest build of the Eclipse SDK here especially the WoA zip package. Just unzip it and run the Eclipse IDE, put it in dark mode and you'll see the lighted scrollbars in the IDE. This latest build contains your original commit for the os_custom.c file.

To see if/how it works before, you need to restore your WoA box to before the current (last month's) Windows update, and test it with the same Eclipse SDK mentioned above. You'd see the dark scrollbars as intended.

@chirontt
Copy link
Contributor

can you try with the latest changes? I changed the title of the PR too

I've tried the same steps as in my previous comment, and the scrollbars are still properly in dark mode, with your latest fix. Here's the screenshot of the Bug562043_DarkTableNoHover.java test:

image

@SyntevoAlex
Copy link
Member

@chirontt Just to clarify:

  • Before this PR, on your newest Windows, scrollbars are light
  • With this PR, on your newest Windows, scrollbars are dark once again
    is that correct?

If yes, please run ver in your commandline and give us Windows version number (where it was broken and now once again fixed).

@chirontt
Copy link
Contributor

* Before this PR, on your newest Windows, scrollbars are light

Yes.

* With this PR, on your newest Windows, scrollbars are dark once again
  is that correct?

Yes.

If yes, please run ver in your commandline and give us Windows version number (where it was broken and now once again fixed).

>ver

Microsoft Windows [Version 10.0.22631.3447]

@SyntevoAlex
Copy link
Member

@hmartinez82 to clarify, I'm waiting for you. @chirontt already got the version numbers, see above.

@SyntevoAlex
Copy link
Member

Please squash commits (because separating them does not convey any additional value here), you can use PR title as commit message. Then it can be merged.

@chirontt
Copy link
Contributor

@SyntevoAlex , @hmartinez82 Sorry to sound like a broken record again, but the new Windows update this week (last Tuesday, to be precise) breaks this os_custom.c file again for WoA. The current WoA version on my box is now:

>ver

Microsoft Windows [Version 10.0.22631.3593]

and running the Bug562043_DarkTableNoHover.java test on it now shows the light scrollbars in dark mode:

image

I've checked the file date of uxtheme.dll and surely enough, it was updated last Tuesday:

>dir \Windows\System32\uxtheme.dll
 Volume in drive C has no label.
 Volume Serial Number is 7EBE-F3EB

 Directory of C:\Windows\System32

05/14/2024  08:01 PM         1,223,680 uxtheme.dll
               1 File(s)      1,223,680 bytes
               0 Dir(s)  96,054,374,400 bytes free

@hmartinez82
Copy link
Contributor Author

hmartinez82 commented May 17, 2024

The argument changed again, it's 0xBF8 now in SetPreferredAppMode()

00007FFD7EE92890 D503237F             pacibsp  
00007FFD7EE92894 F81F0FF3             str         x19,[sp,#-0x10]!  
00007FFD7EE92898 A9BF7BFD             stp         fp,lr,[sp,#-0x10]!  
00007FFD7EE9289C 910003FD             mov         fp,sp  
00007FFD7EE928A0 F00007E8             adrp        x8,wil::details::g_enabledStateManager+40h (07FFD7EF91000h)  
00007FFD7EE928A4 B94BF913             ldr         w19,[x8,#0xBF8]  
00007FFD7EE928A8 2A0003E1             mov         w1,w0  
00007FFD7EE928AC 912FE100             add         x0,x8,#0xBF8  
00007FFD7EE928B0 97FF7E70             bl          _InterlockedExchange (07FFD7EE72270h)  
00007FFD7EE928B4 2A1303E0             mov         w0,w19  
00007FFD7EE928B8 A8C17BFD             ldp         fp,lr,[sp],#0x10  
00007FFD7EE928BC F84107F3             ldr         x19,[sp],#0x10  
00007FFD7EE928C0 D50323FF             autibsp  
00007FFD7EE928C4 D65F03C0             ret

I think that chasing this value is not a good idea. It keeps changing, it seems very unstable across builds of uxtheme.dll. The line above, which deals with the global value has been stable across all the changes we've seen so far. The code detection for Intel is doing exactly that. See

(functionPtr[0x00] == 0x8B) && (functionPtr[0x01] == 0x05) && // mov eax,dword ptr [uxtheme!g_preferredAppMode]

What if I do what I did originally ? And also add a test for the location of the ret instruction? That's what's being done for Intel x64 too.

@hmartinez82
Copy link
Contributor Author

@chirontt Try with the change from 1 min ago.

@chirontt
Copy link
Contributor

@chirontt Try with the change from 1 min ago.

Yes, it works now, i.e. the scrollbars are now dark:

image

@SyntevoAlex
Copy link
Member

SyntevoAlex commented May 17, 2024

The code detection for Intel is doing exactly that

This is not right, it checks a lot more: it checks that two instructions mention the SAME global variable.

const DWORD varOffset1 = *(const DWORD*)(functionPtr + 0x02);
const DWORD varOffset2 = *(const DWORD*)(functionPtr + 0x08);
if (varOffset1 != (varOffset2 + 6))
return FALSE;

@hmartinez82
Copy link
Contributor Author

hmartinez82 commented May 17, 2024

Given the three disassembles we've seen so far, what do you recommend?

00007FFB16C33E10 D503237F             pacibsp  
00007FFB16C33E14 F81F0FF3             str         x19,[sp,#-0x10]!  
00007FFB16C33E18 A9BF7BFD             stp         fp,lr,[sp,#-0x10]!  
00007FFB16C33E1C 910003FD             mov         fp,sp  
00007FFB16C33E20 D00007E8             adrp        x8,wil::details::g_enabledStateManager+40h (07FFB16D31000h)  
00007FFB16C33E24 B94BD913             ldr         w19,[x8,#0xBD8]  
00007FFB16C33E28 2A0003E1             mov         w1,w0  
00007FFB16C33E2C 912F6100             add         x0,x8,#0xBD8  
00007FFB16C33E30 97FF7910             bl          _InterlockedExchange (07FFB16C12270h)  
00007FFB16C33E34 2A1303E0             mov         w0,w19  
00007FFB16C33E38 A8C17BFD             ldp         fp,lr,[sp],#0x10  
00007FFB16C33E3C F84107F3             ldr         x19,[sp],#0x10  
00007FFB16C33E40 D50323FF             autibsp  
00007FFB16C33E44 D65F03C0             ret  
00007FFA2A1D3E10 D503237F             pacibsp  
00007FFA2A1D3E14 F81F0FF3             str         x19,[sp,#-0x10]!  
00007FFA2A1D3E18 A9BF7BFD             stp         fp,lr,[sp,#-0x10]!  
00007FFA2A1D3E1C 910003FD             mov         fp,sp  
00007FFA2A1D3E20 B00007E8             adrp        x8,wil::details::g_enabledStateManager+40h (07FFA2A2D0000h)  
00007FFA2A1D3E24 B94BB913             ldr         w19,[x8,#0xBB8]  
00007FFA2A1D3E28 2A0003E1             mov         w1,w0  
00007FFA2A1D3E2C 912EE100             add         x0,x8,#0xBB8  
00007FFA2A1D3E30 97FF7910             bl          _InterlockedExchange (07FFA2A1B2270h)  
00007FFA2A1D3E34 2A1303E0             mov         w0,w19  
00007FFA2A1D3E38 A8C17BFD             ldp         fp,lr,[sp],#0x10  
00007FFA2A1D3E3C F84107F3             ldr         x19,[sp],#0x10  
00007FFA2A1D3E40 D50323FF             autibsp  
00007FFA2A1D3E44 D65F03C0             ret
00007FFD7EE92890 D503237F             pacibsp  
00007FFD7EE92894 F81F0FF3             str         x19,[sp,#-0x10]!  
00007FFD7EE92898 A9BF7BFD             stp         fp,lr,[sp,#-0x10]!  
00007FFD7EE9289C 910003FD             mov         fp,sp  
00007FFD7EE928A0 F00007E8             adrp        x8,wil::details::g_enabledStateManager+40h (07FFD7EF91000h)  
00007FFD7EE928A4 B94BF913             ldr         w19,[x8,#0xBF8]  
00007FFD7EE928A8 2A0003E1             mov         w1,w0  
00007FFD7EE928AC 912FE100             add         x0,x8,#0xBF8  
00007FFD7EE928B0 97FF7E70             bl          _InterlockedExchange (07FFD7EE72270h)  
00007FFD7EE928B4 2A1303E0             mov         w0,w19  
00007FFD7EE928B8 A8C17BFD             ldp         fp,lr,[sp],#0x10  
00007FFD7EE928BC F84107F3             ldr         x19,[sp],#0x10  
00007FFD7EE928C0 D50323FF             autibsp  
00007FFD7EE928C4 D65F03C0             ret

@SyntevoAlex
Copy link
Member

Here's the diff

D503237F pacibsp
F81F0FF3 str  x19,[sp,#-0x10]!  
A9BF7BFD stp  fp,lr,[sp,#-0x10]!  
910003FD mov  fp,sp  
D00007E8 adrp x8,wil::details::g_enabledStateManager+40h (07FFB16D31000h)  
B94BD913 ldr  w19,[x8,#0xBD8]  
2A0003E1 mov  w1,w0  
912F6100 add  x0,x8,#0xBD8  
97FF7910 bl   _InterlockedExchange (07FFB16C12270h)  
2A1303E0 mov  w0,w19  
A8C17BFD ldp  fp,lr,[sp],#0x10  
F84107F3 ldr  x19,[sp],#0x10  
D50323FF autibsp  
D65F03C0 ret  

........ .......
........ ...  ................
........ ...  ..................
........ ...  .....
........ .... .......................................... (07FFA2A2D0000h)
........ ...  ........#0xBB8.
........ ...  .....  
........ ...  ......#0xBB8  
........ ..   .................... (07FFA2A1B2270h)  
........ ...  ......  
........ ...  ................  
........ ...  ..............  
........ .......  
........ ...

........ .......  
........ ...  ................  
........ ...  ..................  
........ ...  .....  
........ .... .......................................... (07FFD7EF91000h)  
........ ...  ........#0xBF8.
........ ...  .....  
........ ...  ......#0xBF8  
........ ..   .................... (07FFD7EE72270h)  
........ ...  ......  
........ ...  ................  
........ ...  ..............  
........ .......  
........ ...

@SyntevoAlex
Copy link
Member

It can be seen that only a few bytes in the function change. The rest can be verified.

@hmartinez82
Copy link
Contributor Author

hmartinez82 commented May 17, 2024

Ok. I'll add a check for the location of the InterlockExchange call (relative to the beginning of the function) instruction That, with the location of the adrp x8 instruction and the location of the ret instruction should be enough?

@SyntevoAlex
Copy link
Member

I'll add a check for the location of the InterlockExchange call (relative to the beginning of the function)

Not sure what you mean.

To me, it makes sense to validate this part

ldr  w19,[x8,X1]
mov  w1,w0 
add  x0,x8,X2
bl   .................... ................  
mov  w0,w19

Where also X1 == X2

@hmartinez82
Copy link
Contributor Author

@chirontt Can you give it a try with this latest change?

@chirontt
Copy link
Contributor

@chirontt Can you give it a try with this latest change?

It's still working good, i.e. the scrollbars in WoA are still dark:

image

@SyntevoAlex SyntevoAlex merged commit 8c211ce into eclipse-platform:master May 21, 2024
8 of 10 checks passed
@SyntevoAlex
Copy link
Member

In master now, thanks for your contribution.
Today is the last day before the RC1 freeze, and the change only affects barely used arm64 platform, so it should be good.

@hmartinez82 hmartinez82 deleted the oscommon_aarch64_again branch May 21, 2024 07:50
@chirontt
Copy link
Contributor

Thanks @SyntevoAlex , @hmartinez82 for this update. I hope this will keep things smooth for now, until MS decides to spoil the party again.

But I notice that tonight's I-build doesn't contain a recompile of the SWT natives due to this merge. @HannesWell does the automated recompile process require a version increment in order to trigger it?

@HannesWell
Copy link
Member

HannesWell commented May 22, 2024

Thank you @SyntevoAlex for your help.

But I notice that tonight's I-build doesn't contain a recompile of the SWT natives due to this merge. @HannesWell does the automated recompile process require a version increment in order to trigger it?

No that's not necessary, the build just timed-out/failed. Probably due to heavy load on the build servers due to I-build tests.
I restarted the master build again, the next I-build after it succeeds should contain the recompiled natives.

@HannesWell
Copy link
Member

Actually the problem is that the rie8t-win11-arm64 node is offline in the RelEng Jenkins instance and that node is used to compile the binaries for win32.aarch64. Created an help-desk issue:
https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4657

@HannesWell
Copy link
Member

Actually the problem is that the rie8t-win11-arm64 node is offline in the RelEng Jenkins instance and that node is used to compile the binaries for win32.aarch64.

The node is back online and the master build passed before this noon's I-build so it should contain the recompiled binaries:
https://download.eclipse.org/eclipse/downloads/drops4/I20240522-0600/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants