more-f18-changes #153

Closed

Crimson-Hawk wants to merge 10 commits from more-f18-changes into dev

Crimson-Hawk commented

2024-04-15 02:03:03 +02:00

Owner

No description provided.

Crimson-Hawk added 10 commits 2024-04-15 02:03:03 +02:00

5002fde374 Fix bug in ConvertToKMemoryPermission

96db423b41 fix formatting

5f56ba2af6 fix formatting again

3c9732c9d1 fix formatting again again

f91107da04 take in page properties by reference instead of by value

6d13a5ea2b update definition and callsites

bd101a52a0 fix formatting (part 1)

311cfa231b fix formatting (part 2)

codespell / Check for spelling errors (push) Successful in 11s Details

codespell / Check for spelling errors (pull_request) Successful in 11s Details

suyu-ci / Check REUSE Specification (pull_request) Successful in 18s Details

suyu verify / Verify Format (pull_request) Successful in 1m18s Details

suyu verify / test build (linux-mingw, windows) (pull_request) Failing after 2m49s Details

suyu verify / test build (linux-fresh, clang) (pull_request) Failing after 4m10s Details

suyu verify / test build (linux-fresh, linux) (pull_request) Failing after 5m1s Details

suyu verify / android (pull_request) Failing after 8m53s Details

6d20086742 missed a semicolon

codespell / Check for spelling errors (push) Successful in 10s Details

codespell / Check for spelling errors (pull_request) Successful in 12s Details

suyu-ci / Check REUSE Specification (pull_request) Successful in 11s Details

suyu verify / Verify Format (pull_request) Successful in 1m56s Details

suyu verify / test build (linux-fresh, clang) (pull_request) Failing after 4m41s Details

suyu verify / test build (linux-fresh, linux) (pull_request) Failing after 4m59s Details

suyu verify / test build (linux-mingw, windows) (pull_request) Failing after 4m9s Details

suyu verify / android (pull_request) Failing after 13m40s Details

81959ea8e5 fix wrong memory permission

chaphidoesstuff requested review from AMA25 2024-04-18 19:06:30 +02:00

chaphidoesstuff added the

Needs Approvals

label 2024-04-18 19:06:35 +02:00

Exverge commented

2024-04-19 00:10:39 +02:00

Member

@chaphidoesstuff requested review from AMA25

you do realize ama isn't working on the project anymore right? or was this a misclick

> @chaphidoesstuff requested review from AMA25 you do realize ama isn't working on the project anymore right? or was this a misclick

chaphidoesstuff commented

2024-04-19 08:49:40 +02:00

Collaborator

@chaphidoesstuff requested review from AMA25

you do realize ama isn't working on the project anymore right? or was this a misclick

Misclick, sorry

> > @chaphidoesstuff requested review from AMA25 > > you do realize ama isn't working on the project anymore right? or was this a misclick Misclick, sorry

darkshadow commented

2024-04-19 17:38:01 +02:00

First-time contributor

Y'all should skip this PR.

The change in src/core/hle/kernel/k_memory_block.h doesn't compile as is. Even if it is fixed to compile like it is, it's just duplicating what the original code did, but a bit slower since it has to test. The shift in the original code is KMemoryPermission::KernelWrite.

The change in src/core/hle/kernel/k_page_table_base.cpp is problematic as well. It ends up having everything be DisableMergeAttribute::DisableHead where by default in the original it is DisableMergeAttribute::None except in one case. And you can't just change the two lines to be DisableMergeAttribute::None because then you miss that one case.

Y'all should skip this PR. The change in `src/core/hle/kernel/k_memory_block.h` doesn't compile as is. Even if it is fixed to compile like it is, it's just duplicating what the original code did, but a bit slower since it has to test. The shift in the original code _is_ `KMemoryPermission::KernelWrite`. The change in `src/core/hle/kernel/k_page_table_base.cpp` is problematic as well. It ends up having everything be `DisableMergeAttribute::DisableHead` where by default in the original it is `DisableMergeAttribute::None` except in one case. And you can't just change the two lines to be `DisableMergeAttribute::None` because then you miss that one case.

Exverge commented

2024-04-20 01:37:38 +02:00

Member

Y'all should skip this PR.

The change in src/core/hle/kernel/k_memory_block.h doesn't compile as is. Even if it is fixed to compile like it is, it's just duplicating what the original code did, but a bit slower since it has to test. The shift in the original code is KMemoryPermission::KernelWrite.

The change in src/core/hle/kernel/k_page_table_base.cpp is problematic as well. It ends up having everything be DisableMergeAttribute::DisableHead where by default in the original it is DisableMergeAttribute::None except in one case. And you can't just change the two lines to be DisableMergeAttribute::None because then you miss that one case.

This fix was in development and never finished (@nullequal left after the discord was taken down), and this PR is just the unfinished work, hence why it has issues.

> Y'all should skip this PR. > > The change in `src/core/hle/kernel/k_memory_block.h` doesn't compile as is. Even if it is fixed to compile like it is, it's just duplicating what the original code did, but a bit slower since it has to test. The shift in the original code _is_ `KMemoryPermission::KernelWrite`. > > The change in `src/core/hle/kernel/k_page_table_base.cpp` is problematic as well. It ends up having everything be `DisableMergeAttribute::DisableHead` where by default in the original it is `DisableMergeAttribute::None` except in one case. And you can't just change the two lines to be `DisableMergeAttribute::None` because then you miss that one case. This fix was in development and never finished (@nullequal left after the discord was taken down), and this PR is just the unfinished work, hence why it has issues.

darkshadow commented

2024-04-20 05:24:21 +02:00

First-time contributor

This fix was in development and never finished (@nullequal left after the discord was taken down), and this PR is just the unfinished work, hence why it has issues.

Yes, I know. But for the first file I mentioned, the change trying to be made has the exact same outcome as the original code. I can see where one would think there might be a problem there, but the intended change (to have it add KMemoryPermission::KernelWrite) isn't needed. The original code is actually adding that. So, not really any need to change it.

The changes in the second file are really just adding complexity unless some more calls to KPageTableBase::AllocateAndMapPagesImpl was planned. But the changes introduced a bug (already mentioned). If the intended change was actually to change the merge attribute, it could have been done in KPageTableBase::AllocateAndMapPagesImpl without moving things around.

> > This fix was in development and never finished (@nullequal left after the discord was taken down), and this PR is just the unfinished work, hence why it has issues. Yes, I know. But for the first file I mentioned, the change trying to be made has the exact same outcome as the original code. I can see where one would think there might be a problem there, but the intended change (to have it add `KMemoryPermission::KernelWrite`) isn't needed. The original code is actually adding that. So, not really any need to change it. The changes in the second file are really just adding complexity _unless_ some more calls to `KPageTableBase::AllocateAndMapPagesImpl` was planned. But the changes introduced a bug (already mentioned). If the intended change was actually to change the merge attribute, it could have been done in `KPageTableBase::AllocateAndMapPagesImpl` without moving things around.