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.

Sign up for GitHub

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

Jump to bottom

[Linux] xenia-cpu-ppc-tests #803

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[Linux] xenia-cpu-ppc-tests #803

wants to merge 8 commits into from

Conversation

uytvbn
Copy link
Contributor

@uytvbn uytvbn commented Oct 24, 2017

Hi,

This pull request fixes #670 (and also partially #356). I've tried to change as little as possible in Windows code.

$ ./build/bin/Linux/Debug/xenia-cpu-ppc-tests --log_file stdout
...
i> 0000293A Total tests: 1457
i> 0000293A Passed: 1457
i> 0000293A Failed: 0

Comments:

  • src/xenia/base/exception_handler_posix.cc - straightforward implementation based almost entirely on Windows version,
  • src/xenia/base/math.h - off-by-one error, ffs contrary to _BitScanForward returns one-based index,
  • src/xenia/base/memory_posix.cc - porting this exactly is probably not possible as there are significant differences in virtual memory handling between Windows and Linux, tried to follow the behavior of Windows implementation as closely as possible,
  • src/xenia/base/string_buffer.cc - fixed non-standard behavior that happens to work with MSVC,
  • src/xenia/cpu/backend/x64/x64_backend.cc - conform to Linux calling convention,
  • src/xenia/cpu/backend/x64/x64_sequences.cc - the only change here that affects (code-wise) MSVC, it forces Linux to pass XMM values by pointer which is expected by codegen (default calling convention on Linux uses XMM registers), I've checked and it doesn't affect code generated by Visual Studio 2017, could probably hide it behind a typedef if you want to feel extra safe - though you might end up with different types for input and output,
  • src/xenia/cpu/hir/value.cc - fixes Value::MulHi for 64 bit values, returned low not high qword,
  • xenia-build - changes binutils path to use the one you get when building in third-party/binutils.

Thanks.

@uytvbn
Copy link
Contributor Author

uytvbn commented Oct 25, 2017

Reformatted and rebased. It looks like I should have really used clang-3.8 before.

@DrChat
Copy link
Member

DrChat commented Jan 22, 2018

Thanks for the PR! This looks pretty exciting - give me some time and I'll review it later.

hbirchtree added a commit to hbirchtree/xenia that referenced this pull request Jun 3, 2018
@hbirchtree
[Linux] Add some fixes from PR xenia-project#803
a679183
DrChat
DrChat reviewed Oct 12, 2018
View reviewed changes
src/xenia/base/math.h
@@ -203,13 +203,21 @@ inline bool bit_scan_forward(uint64_t v, uint32_t* out_first_set_index) {
#else
inline bool bit_scan_forward(uint32_t v, uint32_t* out_first_set_index) {
int i = ffs(v);
*out_first_set_index = i;
return i != 0;
if (i == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For brevity, you could use the ternary operator (*out_first_set_index = i == 0 ? 0 : i - 1;)

src/xenia/base/math.h
}
inline bool bit_scan_forward(uint64_t v, uint32_t* out_first_set_index) {
int i = ffsll(v);
*out_first_set_index = i;
return i != 0;
if (i == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

src/xenia/base/memory_posix.cc
@@ -61,19 +68,34 @@ bool QueryProtect(void* base_address, size_t& length, PageAccess& access_out) {

FileMappingHandle CreateFileMappingHandle(std::wstring path, size_t length,
PageAccess access, bool commit) {
return nullptr;
int fd = open("/dev/zero", O_RDWR);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you ignore the requested access mode here (O_RDWR)
Not too big of a deal here, I can update this with my implementation.

src/xenia/cpu/backend/x64/x64_backend.cc
@@ -518,24 +569,47 @@ GuestToHostThunk X64ThunkEmitter::EmitGuestToHostThunk() {
extern "C" uint64_t ResolveFunction(void* raw_context, uint32_t target_address);

ResolveFunctionThunk X64ThunkEmitter::EmitResolveFunctionThunk() {
// ebx = target PPC address
// rcx = context
// ebx = target PPC address
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something happened to the spacing here.

src/xenia/cpu/hir/value.cc
static_cast<unsigned __int128>(other->constant.i64));
(static_cast<unsigned __int128>(constant.u64) *
static_cast<unsigned __int128>(other->constant.u64)) >>
64);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@uytvbn
Copy link
Contributor Author

uytvbn commented Oct 14, 2018

Thank you for the review. I'm sorry but I won't be able to continue working on this. Unfortunately I'm swamped right now and it doesn't look like it will change anytime soon. Please just take whatever, if anything, you find useful.

@DrChat
Copy link
Member

DrChat commented Oct 14, 2018

No problem. I'll work on this as I find the time and energy. Appreciate the contribution!

@Guy1524 Guy1524 mentioned this pull request Jan 12, 2019
Open
@bwrsandman bwrsandman mentioned this pull request Mar 9, 2019
Open
@Margen67 Margen67 added the tests label Jul 23, 2019
@bwrsandman bwrsandman mentioned this pull request Aug 14, 2019
Open
17 tasks
@amessier amessier mentioned this pull request May 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DrChat DrChat DrChat left review comments

Assignees
No one assigned
Labels
platform-linux tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

xenia-cpu-ppc-tests Linux Support
4 participants
@uytvbn @DrChat @gibbed @Margen67