Skip to content

Commit

Permalink
vita3k: some minor bugfixes
Browse files Browse the repository at this point in the history
kernel/thread: fix possible crash in log_stack_traceback
modules/SceLibKernel: more correct sceClibSnprintf
kernel/load_self: fix possible crash on incorrect library modules. load process params from first loaded module (eboot.bin) only
modules/SceGxm: fix crash if display_thread not found
kernel/debugger: remove direct access to mem.memory to support memory mapping more complete
vita3k: remove direct access to kernel.threads (it's not thread-safe)
vita3k: implement and fix virtual destructors for class hierarchies to prevent memory leaks (except screen_filters)
clang-tidy warning: Constructing string from nullptr is undefined behaviour
clang warning: use after std::move
  • Loading branch information
bookmist committed Feb 12, 2024
1 parent aa2c1cb commit 6ce9b65
Show file tree
Hide file tree
Showing 16 changed files with 37 additions and 33 deletions.
4 changes: 2 additions & 2 deletions vita3k/codec/include/codec/state.h
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ struct PCMDecoderState : public DecoderState {
bool receive(uint8_t *data, DecoderSize *size) override;

explicit PCMDecoderState(const float dest_frequency);
~PCMDecoderState();
~PCMDecoderState() override;
};

struct AacDecoderState : public DecoderState {
Expand All @@ -221,7 +221,7 @@ struct AacDecoderState : public DecoderState {
uint32_t get_es_size() override;

explicit AacDecoderState(uint32_t sample_rate, uint32_t channels);
~AacDecoderState();
~AacDecoderState() override;
};

struct PlayerState {
Expand Down
2 changes: 1 addition & 1 deletion vita3k/config/include/config/state.h
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ struct Config : YamlLoader {

case _INVALID:
default: {
return nullptr;
return {};
}
}
#undef SWITCH_NAMES
Expand Down
7 changes: 4 additions & 3 deletions vita3k/gdbstub/src/gdb.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ static void modify_reg(CPUState &state, uint32_t reg, uint32_t value) {
}

static std::string cmd_read_registers(EmuEnvState &state, PacketCommand &command) {
const auto guard = std::lock_guard(state.kernel.mutex);
if (state.gdb.current_thread == -1
|| !state.kernel.threads.contains(state.gdb.current_thread))
return "E00";
Expand Down Expand Up @@ -364,7 +365,7 @@ static std::string cmd_read_memory(EmuEnvState &state, PacketCommand &command) {
std::stringstream stream;

for (uint32_t a = 0; a < length; a++) {
stream << fmt::format("{:0>2x}", static_cast<uint8_t>(state.mem.memory[address + a]));
stream << fmt::format("{:0>2x}", *Ptr<uint8_t>(address + a).get(state.mem));
}

return stream.str();
Expand All @@ -385,7 +386,7 @@ static std::string cmd_write_memory(EmuEnvState &state, PacketCommand &command)
return "EAA";

for (uint32_t a = 0; a < length; a++) {
state.mem.memory[address + a] = static_cast<uint8_t>(parse_hex(hex_data.substr(a * 2, 2)));
*Ptr<uint8_t>(address + a).get(state.mem) = static_cast<uint8_t>(parse_hex(hex_data.substr(a * 2, 2)));
}

return "OK";
Expand All @@ -408,7 +409,7 @@ static std::string cmd_write_binary(EmuEnvState &state, PacketCommand &command)
return "EAA";

for (uint32_t a = 0; a < length; a++) {
state.mem.memory[address + a] = data[a];
*Ptr<uint8_t>(address + a).get(state.mem) = data[a];
}

return "OK";
Expand Down
4 changes: 2 additions & 2 deletions vita3k/gui/src/license_install_dialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ void draw_license_install_dialog(GuiState &gui, EmuEnvState &emuenv) {
fs::remove(fs::path(license_path.native()));
delete_license_file = false;
}
license_path = nullptr;
license_path = "";
gui.file_menu.license_install_dialog = false;
state.clear();
}
Expand All @@ -129,7 +129,7 @@ void draw_license_install_dialog(GuiState &gui, EmuEnvState &emuenv) {
ImGui::SetCursorPos(ImVec2(POS_BUTTON, ImGui::GetWindowSize().y - BUTTON_SIZE.y - (20.f * SCALE.y)));
if (ImGui::Button(common["ok"].c_str(), BUTTON_SIZE)) {
gui.file_menu.license_install_dialog = false;
license_path = nullptr;
license_path = "";
state.clear();
}
}
Expand Down
2 changes: 1 addition & 1 deletion vita3k/gui/src/threads_dialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
namespace gui {

void draw_thread_details_dialog(GuiState &gui, EmuEnvState &emuenv) {
ThreadStatePtr &thread = emuenv.kernel.threads[gui.thread_watch_index];
ThreadStatePtr thread = emuenv.kernel.get_thread(gui.thread_watch_index);
CPUState &cpu = *thread->cpu;

ImGui::Begin("Thread Viewer", &gui.debug_menu.thread_details_dialog);
Expand Down
4 changes: 2 additions & 2 deletions vita3k/io/src/io.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -889,10 +889,10 @@ SceUID create_overlay(IOState &io, SceFiosProcessOverlay *fios_overlay) {
// lower order first and in case of equality, last one inserted first
while (overlay_index < io.overlays.size() && overlay.order < io.overlays[overlay_index].order)
overlay_index++;

auto res = overlay.id;
io.overlays.insert(io.overlays.begin() + overlay_index, std::move(overlay));

return overlay.id;
return res;
}

std::string resolve_path(IOState &io, const char *input, const bool is_write, const SceUInt32 min_order, const SceUInt32 max_order) {
Expand Down
14 changes: 7 additions & 7 deletions vita3k/kernel/src/debugger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ void Debugger::add_breakpoint(MemState &mem, uint32_t addr, bool thumb_mode) {
Breakpoint bk;
bk.thumb_mode = thumb_mode;
if (thumb_mode) {
std::memcpy(&bk.data, &mem.memory[addr], sizeof(THUMB_BREAKPOINT));
std::memcpy(&mem.memory[addr], THUMB_BREAKPOINT, sizeof(THUMB_BREAKPOINT));
std::memcpy(&bk.data, Ptr<uint8_t>(addr).get(mem), sizeof(THUMB_BREAKPOINT));
std::memcpy(Ptr<uint8_t>(addr).get(mem), THUMB_BREAKPOINT, sizeof(THUMB_BREAKPOINT));
} else {
std::memcpy(&bk.data, &mem.memory[addr], sizeof(ARM_BREAKPOINT));
std::memcpy(&mem.memory[addr], ARM_BREAKPOINT, sizeof(ARM_BREAKPOINT));
std::memcpy(&bk.data, Ptr<uint8_t>(addr).get(mem), sizeof(ARM_BREAKPOINT));
std::memcpy(Ptr<uint8_t>(addr).get(mem), ARM_BREAKPOINT, sizeof(ARM_BREAKPOINT));
}
breakpoints.emplace(addr, bk);
parent.invalidate_jit_cache(addr, 4);
Expand All @@ -47,7 +47,7 @@ void Debugger::remove_breakpoint(MemState &mem, uint32_t addr) {
const auto lock = std::lock_guard(mutex);
if (breakpoints.contains(addr)) {
auto last = breakpoints[addr];
std::memcpy(&mem.memory[addr], &last.data, last.thumb_mode ? sizeof(THUMB_BREAKPOINT) : sizeof(ARM_BREAKPOINT));
std::memcpy(Ptr<uint8_t>(addr).get(mem), &last.data, last.thumb_mode ? sizeof(THUMB_BREAKPOINT) : sizeof(ARM_BREAKPOINT));
breakpoints.erase(addr);
parent.invalidate_jit_cache(addr, 4);
}
Expand Down Expand Up @@ -90,7 +90,7 @@ void Debugger::add_trampoile(MemState &mem, uint32_t addr, bool thumb_mode, Tram
}

// Create trampoline body
uint32_t *trampoline_insts = reinterpret_cast<uint32_t *>(&mem.memory[trampoline_addr]);
uint32_t *trampoline_insts = Ptr<uint32_t>(trampoline_addr).get(mem);
trampoline_insts[0] = back_inst; // original instruction; if thumb16 it's nop + original thumb16 instruction
trampoline_insts[1] = thumb_mode ? 0xDF53BF00 : 0xEF000053; // SVC 0x53
Trampoline **trampoline_host_ptr = Ptr<Trampoline *>(trampoline_addr + 8).get(mem); // interrupt handler will read this
Expand All @@ -113,7 +113,7 @@ void Debugger::remove_trampoline(MemState &mem, uint32_t addr) {
std::lock_guard<std::mutex> lock(mutex);
const auto it = trampolines.find(addr);
if (it != trampolines.end()) {
uint32_t *insts = reinterpret_cast<uint32_t *>(&mem.memory[addr]);
uint32_t *insts = Ptr<uint32_t>(addr).get(mem);
insts[0] = it->second->original;
trampolines.erase(it);
parent.invalidate_jit_cache(addr, 4);
Expand Down
2 changes: 1 addition & 1 deletion vita3k/kernel/src/kernel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ Ptr<Ptr<void>> KernelState::get_thread_tls_addr(MemState &mem, SceUID thread_id,
Ptr<Ptr<void>> address(0);
// magic numbers taken from decompiled source. There is 0x400 unused bytes of unknown usage
if (key <= 0x100 && key >= 0) {
const ThreadStatePtr thread = util::find(thread_id, threads);
const ThreadStatePtr thread = get_thread(thread_id);
address = thread->tls.get_ptr<Ptr<void>>() + key;
} else {
LOG_ERROR("Wrong tls slot index. TID:{} index:{}", thread_id, key);
Expand Down
7 changes: 3 additions & 4 deletions vita3k/kernel/src/load_self.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,8 @@ static bool load_var_exports(const uint32_t *nids, const Ptr<uint32_t> *entries,
const Ptr<uint32_t> entry = entries[i];

if (nid == NID_PROCESS_PARAM) {
kernel.load_process_param(mem, entry);
if (!kernel.process_param)
kernel.load_process_param(mem, entry);
LOG_DEBUG("\tNID {} (SCE_PROC_PARAMS) at {}", log_hex(nid), log_hex(entry.address()));
continue;
}
Expand Down Expand Up @@ -363,7 +364,6 @@ static bool load_exports(SceKernelModuleInfo *kernel_module_info, const sce_modu
if (!load_func_exports(kernel_module_info, nids, entries, exports->num_syms_funcs, kernel)) {
return false;
}

const auto var_count = exports->num_syms_vars;

if (kernel.debugger.log_exports && var_count > 0) {
Expand Down Expand Up @@ -651,15 +651,13 @@ SceUID load_self(KernelState &kernel, MemState &mem, const void *self, const std
sceKernelModuleInfo->state = module_info->type;

LOG_INFO("Linking SELF {}...", self_path);

if (!load_exports(sceKernelModuleInfo.get(), *module_info, module_info_segment_address, kernel, mem)) {
return -1;
}

if (!load_imports(*module_info, module_info_segment_address, segment_reloc_info, kernel, mem)) {
return -1;
}

const SceUID uid = kernel.get_next_uid();
sceKernelModuleInfo->modid = uid;
{
Expand All @@ -670,5 +668,6 @@ SceUID load_self(KernelState &kernel, MemState &mem, const void *self, const std
const std::lock_guard<std::mutex> guard(kernel.export_nids_mutex);
kernel.module_uid_by_nid.emplace(module_info->module_nid, uid);
}

return uid;
}
12 changes: 7 additions & 5 deletions vita3k/kernel/src/thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -415,10 +415,12 @@ std::string ThreadState::log_stack_traceback() const {
std::stringstream ss;
const Address sp = read_sp(*cpu);
for (Address addr = sp - START_OFFSET; addr <= sp + END_OFFSET; addr += 4) {
const Address value = *Ptr<uint32_t>(addr).get(mem);
const auto mod = kernel.find_module_by_addr(value);
if (mod)
ss << fmt::format("{} (module: {})\n", log_hex(value), mod->module_name);
if (Ptr<uint32_t>(addr).valid(mem)) {
const Address value = *Ptr<uint32_t>(addr).get(mem);
const auto mod = kernel.find_module_by_addr(value);
if (mod)
ss << fmt::format("{} (module: {})\n", log_hex(value), mod->module_name);
}
}
return ss.str();
}
}
2 changes: 1 addition & 1 deletion vita3k/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ int main(int argc, char *argv[]) {
emuenv.app_sku_flag = get_license_sku_flag(emuenv, emuenv.app_info.app_content_id);

if (cfg.console) {
auto main_thread = emuenv.kernel.threads.at(emuenv.main_thread_id);
auto main_thread = emuenv.kernel.get_thread(emuenv.main_thread_id);
auto lock = std::unique_lock<std::mutex>(main_thread->mutex);
main_thread->status_cond.wait(lock, [&]() {
return main_thread->status == ThreadStatus::dormant;
Expand Down
4 changes: 2 additions & 2 deletions vita3k/modules/SceGxm/SceGxm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ static void display_entry_thread(EmuEnvState &emuenv) {
const Address callback_address = emuenv.gxm.params.displayQueueCallback.address();
const ThreadStatePtr display_thread = emuenv.kernel.get_thread(emuenv.gxm.display_queue_thread);
if (!display_thread) {
LOG_CRITICAL("display_thread not found. thid:{}", display_thread->id);
LOG_CRITICAL("display_thread not found. thid:{}", emuenv.gxm.display_queue_thread);
return;
}
Ptr<SceGxmSyncObject> previous_sync = Ptr<SceGxmSyncObject>();
Expand Down Expand Up @@ -2672,7 +2672,7 @@ EXPORT(int, sceGxmInitialize, const SceGxmInitializeParams *params) {
const uint32_t max_queue_size = std::max(std::min(params->displayQueueMaxPendingCount, 3U) - 1, 1U);
emuenv.gxm.display_queue.maxPendingCount_ = max_queue_size;

const ThreadStatePtr main_thread = util::find(thread_id, emuenv.kernel.threads);
const ThreadStatePtr main_thread = emuenv.kernel.get_thread(thread_id);
const ThreadStatePtr display_queue_thread = emuenv.kernel.create_thread(emuenv.mem, "SceGxmDisplayQueue", Ptr<void>(0), SCE_KERNEL_HIGHEST_PRIORITY_USER, SCE_KERNEL_THREAD_CPU_AFFINITY_MASK_DEFAULT, SCE_KERNEL_STACK_SIZE_USER_DEFAULT, nullptr);
if (!display_queue_thread) {
return RET_ERROR(SCE_GXM_ERROR_DRIVER);
Expand Down
2 changes: 1 addition & 1 deletion vita3k/modules/SceLibKernel/SceLibKernel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ EXPORT(int, sceClibSnprintf, char *dst, SceSize dst_max_size, const char *fmt, m
return SCE_KERNEL_ERROR_INVALID_ARGUMENT;
}

return SCE_KERNEL_OK;
return result;
}

EXPORT(int, sceClibSnprintfChk) {
Expand Down
1 change: 1 addition & 0 deletions vita3k/shader/include/shader/usse_program_analyzer.h
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ class USSEBaseNode {
USSEBaseNode *get_parent() const {
return parent;
}
virtual ~USSEBaseNode() = default;

std::size_t children_count() const {
return children.size();
Expand Down
2 changes: 1 addition & 1 deletion vita3k/shader/src/spirv_recompiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ struct VertexProgramOutputProperties {
std::uint32_t location;

VertexProgramOutputProperties()
: name(nullptr)
: name("")
, component_count(0)
, location(0) {}

Expand Down
1 change: 1 addition & 0 deletions vita3k/vkutil/include/vkutil/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ class RingBuffer {
uint32_t data_offset = 0;

explicit RingBuffer(vk::BufferUsageFlags usage, const size_t capacity);
virtual ~RingBuffer() = default;
virtual void create() = 0;

// Allocate new data from ring buffer
Expand Down

0 comments on commit 6ce9b65

Please sign in to comment.