TEMP add Wextra

UoB-HPC · dANW34V3R · Jun 4, 2024 · Feb 27, 2024 · Feb 27, 2024 · Feb 28, 2024
commit 822639562c2aea6d1a7092d8232a290d3597919d
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -71,8 +71,8 @@ set(CMAKE_MACOSX_RPATH 1)
 # Enable PIC for libraries
 set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 
-# Enable additional compiler warnings for all targets
-#add_compile_options(-Wall)
+# Enable additional compiler warnings for SimEng only targets
+set(SIMENG_COMPILE_OPTIONS -Wall -pedantic -Werror) #-Wextra
 
 # Disable RTTI for all targets
 add_compile_options($<$<COMPILE_LANGUAGE:CXX>:-fno-rtti>)

diff --git a/src/include/simeng/CoreInstance.hh b/src/include/simeng/CoreInstance.hh
@@ -24,7 +24,7 @@
 // Program used when no executable is provided; counts down from
 // 1024*1024, with an independent `orr` at the start of each branch.
 // TODO unsure of why this is producing a warning when it is used
-[[maybe_unused]] static uint32_t hex_[] = {
+static const uint32_t hex_[] = {
     0x320C03E0,  // orr w0, wzr, #1048576
     0x320003E1,  // orr w0, wzr, #1
     0x71000400,  // subs w0, w0, #1
@@ -76,10 +76,10 @@ class CoreInstance {
   std::shared_ptr<char> getProcessImage() const;
 
   /** Getter for the size of the created process image. */
-  const uint64_t getProcessImageSize() const;
+  uint64_t getProcessImageSize() const;
 
   /* Getter for heap start. */
-  const uint64_t getHeapStart() const;
+  uint64_t getHeapStart() const;
 
  private:
   /** Generate the appropriate simulation objects as parameterised by the

diff --git a/src/include/simeng/arch/aarch64/helpers/float.hh b/src/include/simeng/arch/aarch64/helpers/float.hh
@@ -157,17 +157,29 @@ RegisterValue scvtf_FixedPoint(
  * Returns single value of type D. */
 template <typename D, typename N>
 D fcvtzu_integer(srcValContainer& sourceValues) {
+  // Ensure type of N so that we know behaviour of type conversions
   static_assert((std::is_same<float, N>() || std::is_same<double, N>()) &&
                 "N not of valid type float or double");
-  N input = sourceValues[0].get<N>();
-  D result = static_cast<D>(0);
+  // TODO do we need to static assert D to be uint32 or uint64
+  N input = sourceValues[0].get<N>();  // float
+  D result = static_cast<D>(0);        // uint
 
   // Check for nan and less than 0
   if (!std::isnan(input) && (input > static_cast<N>(0))) {
     if (std::isinf(input)) {
       // Account for Infinity
       result = std::numeric_limits<D>::max();
     } else if (input >= (N)std::numeric_limits<D>::max()) {
+      // max() will be either 4294967295 or 18446744073709551615
+      // Casting to float results in the following (incorrect) values 4294967296
+      // (+1) or 18446744073709551616 (+1)
+      //
+      // Casting to double results in no erroneous conversion.
+
+      //
+      //
+      // the following values 4294967295 or 18446744073709551615
+
       // Account for the source value being larger than the
       // destination register can support
       result = std::numeric_limits<D>::max();

diff --git a/src/include/simeng/config/SimInfo.hh b/src/include/simeng/config/SimInfo.hh
@@ -104,9 +104,7 @@ class SimInfo {
 
   /** A getter function to retrieve whether or not the special files
    * directories should be generated. */
-  static const bool getGenSpecFiles() {
-    return getInstance()->genSpecialFiles_;
-  }
+  static bool getGenSpecFiles() { return getInstance()->genSpecialFiles_; }
 
   /** A utility function to rebuild/construct member variables/classes. For use
    * if the configuration used changes during simulation (e.g. during the

diff --git a/src/include/simeng/kernel/Linux.hh b/src/include/simeng/kernel/Linux.hh
@@ -244,7 +244,7 @@ class Linux {
   /** Return the host directory file descriptor depending on given pathname and
    * virtual dfd given to syscall. If vdfd is AT_FDCWD then AT_FDCWD is returned
    */
-  int64_t getHostDFD(int64_t vdfd, std::string pathname);
+  int64_t getHostDFD(int64_t vdfd);
 
   /** If the given filepath points to a special file, the filepath is replaced
    * to point to the SimEng equivalent. */

diff --git a/src/include/simeng/kernel/LinuxProcess.hh b/src/include/simeng/kernel/LinuxProcess.hh
@@ -68,7 +68,7 @@ class LinuxProcess {
 
   /** Construct a Linux process from region of instruction memory, with the
    * entry point fixed at 0. */
-  LinuxProcess(span<char> instructions,
+  LinuxProcess(span<const uint8_t> instructions,
                ryml::ConstNodeRef config = config::SimInfo::getConfig());
 
   ~LinuxProcess();

diff --git a/src/lib/AlwaysNotTakenPredictor.cc b/src/lib/AlwaysNotTakenPredictor.cc
@@ -2,9 +2,8 @@
 
 namespace simeng {
 
-BranchPrediction AlwaysNotTakenPredictor::predict(uint64_t address,
-                                                  BranchType type,
-                                                  int64_t knownOffset) {
+BranchPrediction AlwaysNotTakenPredictor::predict(
+    [[maybe_unused]] uint64_t address, BranchType type, int64_t knownOffset) {
   return {false, 0};
 }
 

diff --git a/src/lib/CMakeLists.txt b/src/lib/CMakeLists.txt
@@ -58,7 +58,7 @@ target_include_directories(libsimeng PRIVATE ${CMAKE_CURRENT_BINARY_DIR})
 target_link_libraries(libsimeng capstone)
 # TODO TOML check this
 # Only enable compiler warnings for our code
-target_compile_options(libsimeng PRIVATE -Wall -pedantic -Werror)
+target_compile_options(libsimeng PRIVATE ${SIMENG_COMPILE_OPTIONS})
 
 set_target_properties(libsimeng PROPERTIES VERSION ${SimEng_VERSION})
 set_target_properties(libsimeng PROPERTIES SOVERSION ${SimEng_VERSION_MAJOR})

diff --git a/src/lib/CoreInstance.cc b/src/lib/CoreInstance.cc
@@ -96,7 +96,7 @@ void CoreInstance::createProcess(std::string executablePath,
   } else if (assembledSource_) {
     // Create a process image from the source code assembled by LLVM.
     process_ = std::make_unique<kernel::LinuxProcess>(
-        span<char>(source_, sourceSize_), config_);
+        span<const char>(source_, sourceSize_), config_);
     // Raise error if created process is not valid
     if (!process_->isValid()) {
       std::cerr << "[SimEng:CoreInstance] Could not create process based on "
@@ -108,7 +108,8 @@ void CoreInstance::createProcess(std::string executablePath,
     // TODO remove once default binary in use
     // Create a process image from the set of instructions held in hex_
     process_ = std::make_unique<kernel::LinuxProcess>(
-        span<char>(reinterpret_cast<char*>(hex_), sizeof(hex_)), config_);
+        span<const char>(reinterpret_cast<const char*>(hex_), sizeof(hex_)),
+        config_);
 
     // Raise error if created process is not valid
     if (!process_->isValid()) {
@@ -318,12 +319,10 @@ std::shared_ptr<char> CoreInstance::getProcessImage() const {
   return processMemory_;
 }
 
-const uint64_t CoreInstance::getProcessImageSize() const {
+uint64_t CoreInstance::getProcessImageSize() const {
   return processMemorySize_;
 }
 
-const uint64_t CoreInstance::getHeapStart() const {
-  return process_->getHeapStart();
-}
+uint64_t CoreInstance::getHeapStart() const { return process_->getHeapStart(); }
 
 }  // namespace simeng
diff --git a/src/lib/arch/aarch64/ExceptionHandler.cc b/src/lib/arch/aarch64/ExceptionHandler.cc
@@ -718,7 +718,8 @@ bool ExceptionHandler::init() {
          static_cast<uint16_t>(arch.getSystemRegisterTag(ARM64_SYSREG_SVCR))});
     regValues.push_back(RegisterValue(newSVCR, 8));
 
-    ProcessStateChange stateChange = {ChangeType::REPLACEMENT, regs, regValues};
+    ProcessStateChange stateChange = {
+        ChangeType::REPLACEMENT, regs, regValues, {}, {}};
     return concludeSyscall(stateChange);
   }
 

diff --git a/src/lib/arch/aarch64/InstructionMetadata.cc b/src/lib/arch/aarch64/InstructionMetadata.cc
@@ -705,6 +705,7 @@ InstructionMetadata::InstructionMetadata(const cs_insn& insn)
     case Opcode::AArch64_INCP_XP_S:
       operands[0].access = CS_AC_READ | CS_AC_WRITE;
       operands[1].access = CS_AC_READ;
+      break;  // TODO IS THIS THE CORRECT, DOES IT NEED TO FALLTHROUGH?
     case Opcode::AArch64_LD1i32:
       [[fallthrough]];
     case Opcode::AArch64_LD1i64:

diff --git a/src/lib/arch/riscv/ExceptionHandler.cc b/src/lib/arch/riscv/ExceptionHandler.cc
@@ -448,6 +448,7 @@ bool ExceptionHandler::init() {
       case 131: {  // tgkill
         // TODO currently returns success without action
         stateChange = {ChangeType::REPLACEMENT, {R0}, {0}};
+        break;
       }
       case 134: {  // rt_sigaction
         // TODO: Implement syscall logic. Ignored for now as it's assumed the

diff --git a/src/lib/kernel/Linux.cc b/src/lib/kernel/Linux.cc
@@ -45,7 +45,7 @@ void Linux::createProcess(const LinuxProcess& process) {
        "/sys/devices/system/cpu/online", "core_id", "physical_package_id"});
 }
 
-int64_t Linux::getHostDFD(int64_t vdfd, std::string pathname) {
+int64_t Linux::getHostDFD(int64_t vdfd) {
   // -100 = AT_FCWD on linux. Pass back AT_FDCWD for host platform e.g. -2 for
   // MAC
   if (vdfd == -100) {
@@ -150,7 +150,7 @@ int64_t Linux::faccessat(int64_t dfd, const std::string& filename, int64_t mode,
 
   // Get host dirfd. May return -1 in case of no mapping, pass through to host
   // faccessat to deal with this
-  int64_t hostDfd = Linux::getHostDFD(dfd, filename);
+  int64_t hostDfd = Linux::getHostDFD(dfd);
 
   // Pass call through to host
   int64_t retval = ::faccessat(hostDfd, new_pathname.c_str(), mode, flag);
@@ -192,7 +192,7 @@ int64_t Linux::newfstatat(int64_t dfd, const std::string& filename, stat& out,
 
   // Get host dirfd. May return -1 in case of no mapping, pass through to host
   // fstatat to deal with this
-  int64_t hostDfd = Linux::getHostDFD(dfd, filename);
+  int64_t hostDfd = Linux::getHostDFD(dfd);
 
   // Pass call through to host
   struct ::stat statbuf;
@@ -415,8 +415,9 @@ int64_t Linux::munmap(uint64_t addr, size_t length) {
   return 0;
 }
 
-uint64_t Linux::mmap(uint64_t addr, size_t length, int prot, int flags, int fd,
-                     off_t offset) {
+uint64_t Linux::mmap(uint64_t addr, size_t length, [[maybe_unused]] int prot,
+                     [[maybe_unused]] int flags, [[maybe_unused]] int fd,
+                     [[maybe_unused]] off_t offset) {
   LinuxProcessState* lps = &processStates_[0];
   std::shared_ptr<struct vm_area_struct> newAlloc(new vm_area_struct);
   if (addr == 0) {  // Kernel decides allocation
@@ -502,7 +503,7 @@ int64_t Linux::openat(int64_t dfd, const std::string& filename, int64_t flags,
   // Get host dirfd. May return -1 in case of no mapping, pass through to host
   // openat to deal with this
   // TODO should this be new_pathname (resp. filename)
-  int64_t hDfd = Linux::getHostDFD(dfd, filename);
+  int64_t hDfd = Linux::getHostDFD(dfd);
 
   // Pass call through to host
   int64_t hostFd = ::openat(hDfd, new_pathname.c_str(), newFlags, mode);

diff --git a/src/lib/kernel/LinuxProcess.cc b/src/lib/kernel/LinuxProcess.cc
@@ -63,7 +63,8 @@ LinuxProcess::LinuxProcess(const std::vector<std::string>& commandLine,
 }
 
 // TODO can this be marked as only usable by test? or is it used by SST??
-LinuxProcess::LinuxProcess(span<char> instructions, ryml::ConstNodeRef config)
+LinuxProcess::LinuxProcess(span<const uint8_t> instructions,
+                           ryml::ConstNodeRef config)
     : STACK_SIZE(config["Process-Image"]["Stack-Size"].as<uint64_t>()),
       HEAP_SIZE(config["Process-Image"]["Heap-Size"].as<uint64_t>()) {
   // Set program command string to a relative path of "Default"

diff --git a/test/unit/aarch64/ArchitectureTest.cc b/test/unit/aarch64/ArchitectureTest.cc
@@ -79,13 +79,13 @@ class AArch64ArchitectureTest : public testing::Test {
   })YAML");
 
   // fdivr z1.s, p0/m, z1.s, z0.s
-  std::array<uint8_t, 4> validInstrBytes = {0x01, 0x80, 0x8c, 0x65};
+  const std::array<uint8_t, 4> validInstrBytes = {0x01, 0x80, 0x8c, 0x65};
   std::array<uint8_t, 4> invalidInstrBytes = {0x20, 0x00, 0x02, 0x8c};
 
   std::unique_ptr<Architecture> arch;
   kernel::Linux kernel;
   kernel::LinuxProcess process = kernel::LinuxProcess(
-      span((char*)validInstrBytes.data(), validInstrBytes.size()));
+      span(validInstrBytes.data(), validInstrBytes.size()));
 };
 
 TEST_F(AArch64ArchitectureTest, predecode) {