Fix Maybe Uninitialised Error and Remove UB by dANW34V3R · Pull Request #456 · UoB-HPC/SimEng

dANW34V3R · 2025-05-15T13:01:02Z

Fixes #426 among other errors.

Add a safe pointer struct to remove undefined behaviour caused by reinterpret casting in the RegisterValue. The safe pointer hides the underlying pointer to RegisterValue data. This enforces immutability and fixes UB by converting unsafe reinterpret_casts to safe memcpys.

dANW34V3R · 2025-05-15T13:13:25Z

test/regression/aarch64/Syscall.cc


  // Check fstatat returned 0
-  EXPECT_EQ(getGeneralRegister<int64_t>(27), 0);
+  EXPECT_EQ(getGeneralRegister<int64_t>(21), 0);


@FinnWilkinson @jj16791 I think this test was checking the wrong register. Can you double check?

If we are wanting to check the newfstatat register, then it sould be 21 yeah. My sme branch has register 21 being checked though so unsure when this happened...

src/include/simeng/RegisterValue.hh

dANW34V3R · 2025-05-15T16:19:10Z

src/lib/arch/aarch64/ExceptionHandler.cc

+        stateChange.memoryAddressValues.push_back(RegisterValue(
+            reinterpret_cast<const uint8_t*>(out.data()), outSize));


The reinterpret_casting throughout this function is alarming and requires further investigation

Personally I think RegisterValue should have proper move semantics and you should be able to create a blank register of some size and move it into the vector

dANW34V3R · 2025-05-15T16:24:39Z

src/lib/arch/aarch64/ExceptionHandler.cc


            // Write data for this buffer in 128-byte chunks
-            auto iSrc = reinterpret_cast<const char*>(buffers[i].data());
+            auto iSrc = reinterpret_cast<const uint8_t*>(buffers[i].data());


Not sure about this one

dANW34V3R · 2025-05-15T16:25:40Z

src/lib/arch/riscv/ExceptionHandler.cc

Comments same as AArch exception handler

dANW34V3R · 2025-05-15T16:29:06Z

sst/SimEngMemInterface.cc

+  // TODO avoid malloc and copy altogether
+  uint8_t* data = (uint8_t*)malloc(size);
+  aggrReq->data.getAsVector<char>().copyTo(data, size);
+


Could have a member buffer but not sure what size?

dANW34V3R · 2025-05-15T16:32:09Z

src/lib/arch/aarch64/Instruction_execute.cc

+                reinterpret_cast<uint8_t*>(memData.data()), memData.size() * sizeof(uint64_t));
            index++;
            memData.clear();
          }
        }
        // Check if final data needs putting into memoryData_
        if (memData.size() > 0) {
-          memoryData_[index] = RegisterValue((char*)memData.data(),
+          memoryData_[index] = RegisterValue(reinterpret_cast<uint8_t*>(memData.data()),


Could be dangerous

dANW34V3R · 2025-05-15T16:33:08Z

src/lib/arch/aarch64/Instruction_execute.cc

Reinterpret casts throughout need to be checked for safety

ABenC377

Looks good. A couple of general questions (more for my own understanding than anything else).

src/include/simeng/RegisterValue.hh

src/include/simeng/arch/aarch64/helpers/neon.hh

ABenC377 · 2025-05-16T08:51:00Z

src/include/simeng/arch/aarch64/helpers/neon.hh


  for (int i = 0; i < I; i++) {
    if (isXtn2 & (i < (I / 2))) {
+      assert(isXtn2 && "isXtn2 is false so d is not initialised");


Will this assert ever fail? If isXtn2 & anything is true, wont isXtn2 always be true?

This is layover from when I was having issues previously. No harm in keeping it but happy to remove. Your call

I think get rid of it, as it doesn't do anything and so might cause momentary confusion for someone reading this bit of code in the future.

tom91136 · 2025-08-21T12:06:09Z

src/lib/arch/aarch64/ExceptionHandler.cc

  assert(response->data && "unhandled failed read in exception handler");
  uint8_t bytesRead = response->target.size;
-  const uint8_t* data = response->data.getAsVector<uint8_t>();
+  uint8_t* data = (uint8_t*)malloc(bytesRead);


Create a block-scoped std::vector<uint8_t> and copy to that instead of malloc/free

tom91136 · 2025-08-21T12:09:16Z

src/include/simeng/RegisterValue.hh

+    memcpy(dest, ptr, bytes);
+  }
+
+  void copyTo(void* dest, const size_t bytes, const uint64_t offset) {


This method should be const too?

tom91136 · 2025-08-21T12:18:06Z

src/lib/arch/aarch64/ExceptionHandler.cc

+        stateChange.memoryAddressValues.push_back(RegisterValue(
+            reinterpret_cast<const uint8_t*>(out.data()), outSize));


Personally I think RegisterValue should have proper move semantics and you should be able to create a blank register of some size and move it into the vector

tom91136 · 2025-08-21T12:19:28Z

src/lib/arch/riscv/ExceptionHandler.cc

  uint8_t bytesRead = response->target.size;
-  const uint8_t* data = response->data.getAsVector<uint8_t>();
+  // TODO clean up malloc
+  uint8_t* data = (uint8_t*)malloc(bytesRead);


See comment about using scoped vector

tom91136 · 2025-08-21T12:20:35Z

src/lib/kernel/LinuxProcess.cc

  initialStackFrame.push_back(commandLine_.size());  // argc
  for (size_t i = 0; i < commandLine_.size(); i++) {
-    char* argvi = commandLine_[i].data();
+    uint8_t* argvi = reinterpret_cast<uint8_t*>(commandLine_[i].data());


use auto on lhs if rhs is reinterpret_cast

tom91136 · 2025-08-21T12:22:24Z

test/regression/aarch64/AArch64RegressionTest.hh

    return "+sve,+lse";
 #elif SIMENG_LLVM_VERSION < 18
-    return "+sve,+lse,+sve2,+sme,+sme-f64";
+    return "+sve,+lse,+sve2,+sme,+sme-f64,+sme-i64";


Should this be part of the PR?

tom91136 · 2025-08-21T12:23:39Z

src/include/simeng/RegisterValue.hh

 #include <algorithm>
 #include <cassert>
 #include <cstring>
+#include <iostream>


Remove extra header

src/include/simeng/RegisterValue.hh

tom91136 · 2025-08-21T12:33:25Z

src/include/simeng/RegisterValue.hh

+struct safePointer {
+ public:
+  explicit safePointer(const uint8_t* ptr) : ptr(ptr) {}
+  explicit safePointer() : ptr(nullptr) {}


do we need a null safePointer? Is it still safe if calling anything on it is UB? also, might want to assert in the ctor above if the build is debug

dANW34V3R added 13 commits May 12, 2025 17:25

Initial fix for Wmaybe-uninitialized

e77c206

Force clangformat

7292aaf

Hacky fix for failing test

7ec30b0

Add no strict ailiasing flag

9fe3e21

[TEMP] Half updated with wrapper class

6a7fa2f

Temp commit for rebase

71e32dd

Save progress

39b576e

Fix incorrect register in newfstatat test

7ac1fc3

Fix various issues

7383712

Update sme feature list

fe1708d

Use memcpy for non-local values

9f72ace

Minor cleanup

eeb158f

Hide underlying pointer

1688158

github-project-automation bot added this to SimEng Development May 15, 2025

github-project-automation bot moved this to ToDo in SimEng Development May 15, 2025

dANW34V3R added 2 commits May 15, 2025 14:57

Merge branch 'dev' into newUninitialised

fc9b540

Cleanup

ca89844

dANW34V3R commented May 15, 2025

View reviewed changes

Remove reinterpret casts

3e4de99

ABenC377 reviewed May 16, 2025

View reviewed changes

dANW34V3R requested review from FinnWilkinson, JosephMoore25, jj16791 and tom91136 May 16, 2025 10:04

Remove extern

e1c3ed3

tom91136 requested changes Aug 21, 2025

View reviewed changes

github-project-automation bot moved this from ToDo to Changes Requested in SimEng Development Aug 21, 2025

		stateChange.memoryAddressValues.push_back(RegisterValue(
		reinterpret_cast<const uint8_t*>(out.data()), outSize));

Conversation

dANW34V3R commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ABenC377 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants