Update depreciated nxdk functions

Fisherman166 commented 4 years ago

JayFoxRox mentioned in this issue that we needed to stop using depreciated nxdk functions. I have updated all of the depreciated functions to use the equivalent new functions.

JayFoxRox commented 4 years ago

~~There is a typo in the commit message~~ It's actually just british english. Don't mind me.

ergo720 commented 4 years ago

The Travis build is failing with the error use of undeclared identifier 'char16_t/char32_t', so this cannot be merged until that is fixed. Additionally, the GetTickCount issue is still there, but that should probably be corrected in a separate PR.

Fisherman166 commented 4 years ago

I was getting the char16_t char32_t errors as well before updating my clang version locally. I've never touched Travis or any other CI before so I'm hesitant to make the change, but my guess would be we need to add this to the install: section:

- alias clang=clang-7

JayFoxRox commented 4 years ago

Yes, sounds like a clang issue. That proposed fix could work - why wasn't it tried?

You are also running Travis CI on Ubuntu 14.04 which only gets security updates by now. You should probably switch to 18.04 (which has higher default package versions). While nxdk isn't too cutting edge in terms of OS requirements, I assume that we'll pay less attention to 16.04 reports once 20.04 hits. 4 years is a long time in software development, and it's usually free and easy to upgrade.

If you want to overhaul your CI, you could probably also just steal / mirror the nxdk GitHub Actions workflow. I assume it only requires minimal changes, and you could always just adapt the latest upstream changes (thereby avoiding bitrot).

JayFoxRox commented 4 years ago

For your information @Fisherman166 :

It's probably clang-7.0 (not just 7) in the alias.
- echo "deb http://apt.llvm.org/trusty/ llvm-toolchain-trusty-7 main" | sudo tee -a /etc/apt/sources.list needs to be updated as you have moved away from trusty.

Fisherman166 commented 4 years ago

Xenial with alias clang=clang-7 passed build so it seems to be working.

JayFoxRox commented 4 years ago

You should still avoid listing trusty package sources on non-trusty Ubuntus. See http://apt.llvm.org/

Xenial (16.04) is also almost 4 years old, so don't expect support to last too long. Also it looks like Bionic has pre-packaged clang-8 already. So the apt would only be necessary for nightly builds probably. Xenial is stuck on pre-packaged clang-6.

ergo720 commented 4 years ago

I tested these changes with https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/commit/08691c24e17f48e7cecc839fb4a7fd6543d332c5 and compared the test results with the build with tag 20190604141115-c3dc177 and confirmed that they are identical, so this doesn't seem to cause any regressions. I will merge this in a day or so if there are no more remarks.

JayFoxRox commented 4 years ago

I'm surprised to hear that it still works given https://github.com/XboxDev/nxdk-pdclib/pull/15#issuecomment-575432934. I don't think it should be able to open the config file (or the output file) anymore? The Xbox kernel used in our winapi implementation shouldn't be able to load a relative path (whereas Windows probably allows it).

Please retest that on hardware; I believe it only works due to Cxbx-R bugs.

I'm personally not too hyped about hardcoding D:\\ as the application directory (particularly in the libc API). For the winapi I'd consider that more standardish (also because winapi itself already makes some other assumptions about the platform the code will run on, whereas libc is supposed to be portable). So I can't really recommend that as permanent solution (yet), but we also don't have another solution right now either.

Anyhow, I don't think there are many filepaths in Cxbx-R. You should keep an eye on how https://github.com/XboxDev/nxdk-pdclib/pull/15 develops and potential follow-ups in the nxdk winapi lib.

Also a nit about the PR: Commit titles on this repo typically don't end in a period.

Fisherman166 commented 4 years ago

This is what it looks like running on CXBX-R:

[0x36EC] INFO : DBG Trying to open config file: config.txt [0x36EC] INFO : DBG Could not open config file 'config.txt' for read [0x36EC] INFO : DBG Kernel Test Suite: Skipping creating kernel_tests.log because on emulator [0x36EC] INFO : DBG Kernel Test Suite [0x36EC] INFO : DBG Running tests in emulator mode No Specific tests specified. Running all tests (Single Pass).

This is what it looks like on real hardware:

Kernel Test Suite Running tests in real hardware mode No Specific tests specified. Running all tests (Single Pass).

This works because in globals.c we set is_emu = 1 by default. On CXBX-R the config file fails to load but it works fine because is_emu is already set to 1. On real hardware the config file can be opened, so we create a config file that sets is_emu=0 which allows the test suite to work in both environments. Its not a great solution, but its a workaround for the filesystem differences that exist between real hardware and CXBX-R.

ergo720 commented 4 years ago

I'm unable to reproduce your issue, for me it still works fine, even with is_emu=0. I retested with https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/commit/08691c24e17f48e7cecc839fb4a7fd6543d332c5 and a config.txt with is_emu=0 and no tests specified, and the following are the relevant kernel calls extracted from the kernel log generated by Cxbx-R.

[0x2704] INFO : DBG     Trying to open config file: config.txt
[0x2704] DBG     DbgPrint returns 0
[0x2704] NT      NtCreateFile forwarding to "IoCreateFile"...
[0x2704] IO      IoCreateFile(
 OUT FileHandle         : 0x149BFE00
   DesiredAccess        : 80100080
   ObjectAttributes     : 0x149BFDE4 -> OBJECT_ATTRIBUTES* {
   .RootDirectory        : FFFFFFFD
   .ObjectName           : 0x149BFDF8 -> STRING* {
   .Length               : A
   .MaximumLength        : B
   .Buffer               : (char *)0x000397B0 = "config.txt"}
   .Attributes           : 40}
 OUT IoStatusBlock      : 0x149BFDF0
   AllocationSize       : 0x00000000
   FileAttributes       : 80
   ShareAccess          : 1
   Disposition          : (CREATE_DISPOSITION)0x00000001 = FILE_OPEN
   CreateOptions        : (CREATE_OPTION)0x00000020 = FILE_SYNCHRONOUS_IO_NONALERT
   Options              : 0
);
[0x2704] DEBUG: IO      IoCreateFile = 0x00000648
[0x2704] IO      IoCreateFile returns 0
[0x2704] NT      NtQueryInformationFile(
   FileHandle           : 00000648
 OUT IoStatusBlock      : 0x149BFDD8
 OUT FileInformation    : 0x149BFDA0
   Length               : 38
   FileInformationClass : (FILE_INFORMATION_CLASS)0x00000022 = FileNetworkOpenInformation
);
[0x2704] NT      NtQueryInformationFile returns 0
[0x2704] NT      NtReadFile(
   FileHandle           : 00000648
   Event                : 00000000
   ApcRoutine           : 00000000
   ApcContext           : 00000000
 OUT IoStatusBlock      : 0x149BFE00
 OUT Buffer             : 0x14B505F8
   Length               : 8
   ByteOffset           : 0x00000000
);
[0x2704] NT      NtReadFile returns 0
[0x2704] NT      NtClose(
   Handle               : 00000648
);
[0x2704] NT      NtClose returns 0
[0x2704] NT      NtCreateFile forwarding to "IoCreateFile"...
[0x2704] IO      IoCreateFile(
 OUT FileHandle         : 0x149BFE4C
   DesiredAccess        : 40100080
   ObjectAttributes     : 0x149BFE30 -> OBJECT_ATTRIBUTES* {
   .RootDirectory        : FFFFFFFD
   .ObjectName           : 0x149BFE44 -> STRING* {
   .Length               : 10
   .MaximumLength        : 11
   .Buffer               : (char *)0x0003A0E7 = "kernel_tests.log"}
   .Attributes           : 40}
 OUT IoStatusBlock      : 0x149BFE3C
   AllocationSize       : 0x00000000
   FileAttributes       : 80
   ShareAccess          : 2
   Disposition          : (CREATE_DISPOSITION)0x00000005 = FILE_OVERWRITE_IF
   CreateOptions        : (CREATE_OPTION)0x00000020 = FILE_SYNCHRONOUS_IO_NONALERT
   Options              : 0
);
[0x2704] DEBUG: IO      IoCreateFile = 0x00000648
[0x2704] IO      IoCreateFile returns 0
[0x2704] DBG     DbgPrint(
   Format               : (char *)0x000399A2 = "%s"
   "..."                : ...
);
[0x2704] INFO : DBG     Kernel Test Suite
[0x2704] DBG     DbgPrint returns 0
[0x2704] NT      NtWriteFile(
   FileHandle           : 00000648
   Event                : 00000000
   ApcRoutine           : 00000000
   ApcContext           : 00000000
 OUT IoStatusBlock      : 0x149BFC14
   Buffer               : 149BFC94
   Length               : 12
   ByteOffset           : 0x00000000
);
[0x2704] NT      NtWriteFile returns 0
[0x2704] DBG     DbgPrint(
   Format               : (char *)0x000399A2 = "%s"
   "..."                : ...
);
[0x2704] INFO : DBG     Running tests in real hardware mode
[0x2704] DBG     DbgPrint returns 0
[0x2704] NT      NtWriteFile(
   FileHandle           : 00000648
   Event                : 00000000
   ApcRoutine           : 00000000
   ApcContext           : 00000000
 OUT IoStatusBlock      : 0x149BFBF8
   Buffer               : 149BFC78
   Length               : 24
   ByteOffset           : 0x00000000
);
[0x2704] NT      NtWriteFile returns 0
[0x2704] DBG     DbgPrint(
   Format               : (char *)0x000399A2 = "%s"
   "..."                : ...
);
[0x2704] INFO : DBG     No Specific tests specified. Running all tests (Single Pass).

On Cxbx-R, file creation is trapped by NtCreateFile, which simply forwards to IoCreateFile that does the actual work. As you can see from above, both calls for config.txt and kernel_tests.log are succeeding, and I find the kernel_tests.log file in the directory of the test suite xbe with the test results written to it.

RadWolfie commented 4 years ago

@Fisherman166 I can confirm latest Cxbx-Reloaded build is able to read config.txt file just fine.

Few things could cause this to happen: A) Cxbx-Reloaded doesn't have permission to access the file (file was created/saved as admin) B) didn't create the file in same directory where loaded default.xbe is at C) using older cxbxr build (buggy build) D) extended "255" filepath length limitation

Cheers

JayFoxRox commented 4 years ago

I'm surprised this got merged. I'm assuming this broke real hardware support (despite what @Fisherman166 claimed).

I still believe this will not work on a real MS kernel because that doesn't support relative paths (as shown in the comment I linked above, to warn about this). CreateFile with a path of "foobar" will not work on Xbox; it needs to be "D:\\foobar". OpenXDK XCreateFile had a special handler for this, nxdk winapi does not (but it might be added in the future, see the linked discussion).
I still believe that it does work in Cxbx-R (because of inaccurate emulation / missing restrictions in Cxbx-R). CreateFile with a path of "foobar" will work on Windows. I'm assuming that Cxbx-R maps the Xbox CreateFile function to the Windows CreateFile function without explicitly adding the Xbox specific restriction; so your code probably still works in Cxbx-R.

RadWolfie commented 4 years ago

XCreateFile is depreciated from https://github.com/XboxDev/nxdk/pull/150 pull request. It would be better to create an issue or make a pull request to correct this issue for xbox hardware.

I can confirm on xbox hardware with real MS kernel isn't able to read the config.txt file. Since it has been output to screen, no output to a log file, and even set to one test (to see earlier messages) also didn't change the output result.

Cxbx-Reloaded / xbox_kernel_test_suite

Update depreciated nxdk functions #61