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

[WIP] Reimplement vm::ref to replace vm::ptr #12988

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

Conversation

Nekotekina
Copy link
Member

I recently discovered a problem with unaligned memory access of objects stored in PS3 memory. It usually works as intended due to both PS3 and x86 PC intrinsically permitting unaligned reads/writes of 8 bytes and lower, but in fact it's a UB in C++ and it's pure luck it works without problems. One possible solution is to use memcpy instead of creating a native pointer or reference which may be unaligned. This may lead to the deprecation of uncontrollable memory access via native pointers, as they leak deeply into other APIs and may cause various problems, notably having to do the similar workaround in file reads/writes.

@kd-11
Copy link
Contributor

kd-11 commented Nov 27, 2022

Hmm, how about custom wrapped types that are implementation compliant? Though maybe std::memcpy ellision is guaranteed in the spec?

I mean like:

template <typename T>
struct vm_safe_t {
#if ALLOW_UNALIGNED
    T v;
#else
    char v[sizeof T];
#endif

    operator T() const {
#if ALLOW_UNALIGNED
        return v;
#else
        T ret;
        std::memcpy(&ret, v, sizeof T);
        return ret;
#endif
    }
    
    // ...other getters/setters
}

You can then cast pointers from this the same way we do for be_t and use it as a proper type with pointers and such.
You still have to deal with implementation-defined behavior but that is well known and documented information. But then again, I'm used to almost everything on GPU being "implementation defined" so I may be more comfortable with such things.
This (diverging path based on compile-time check) is how this issue is normally worked around in the linux kernel afaik with a kconfig option.

As far as alignment issues, at the moment, the only thing I've seen so far is unaligned atomic writes on arm64. The rest should be fine. No idea about RISC-V, though I plan on looking at this some time down the line if the uarch pans out.

@elad335
Copy link
Contributor

elad335 commented Nov 28, 2022

Yea a flag is good because on debug builds an explicit memcpy call can be invoked which may or may not use atomic loads and depending on the implementation (matters for PPU interpreter for memory instructions).

@Nekotekina
Copy link
Member Author

Not sure about this. PPU interpreter can be forced to use relaxed atomic instructions with ASMJIT "JIT".

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

Successfully merging this pull request may close these issues.

4 participants