Fix macOS libcachesim compilation

[laustam]

Problem

The problem is that the installation of libcachesim fails on macOS due to too many errors caught by Clang that are otherwise ignored by the less strict GCC. This MR addresses issues listed in Issue #182 and fixes further issues encountered along the way.

Issue 1: Code quality CI never triggers

The code-quality.yaml includes a clang-tidy formatter that seemed to not run previously due to its configuration. After ensuring that all C and C++ files are captured, the whole repository was reformatted to conform to clang-tidy. As the repository-wide reformatting affected a lot of files without changing their functionality, the commit is ignored from git blame.

Issue 2: Incorrect format specifiers

A common error reported by Clang was incorrect format specifiers used with the printf function, specifically that an argument of type int64_t should use the %lld format specifier in printf calls. However, %lld behaves differently across platforms because of differences in how data types are defined and how format specifiers are interpreted (on Ubuntu/Linux, int64_t requires the %ld format specifier). The solution for this is to use the format macros defined in <inttypes.h>, specifically PRId64 in this case - this expands to the correct format specifier depending on the platform and ensures that this is portable and has correct formatting across all systems. An example of the error is shown here:

libCacheSim/cache/eviction/LIRS.c:696:53: error: format specifies type 'unsigned long' but the argument has type 'int64_t' (aka 'long long') [-Werror,-Wformat]
    printf("%ld(%lu, %s, %s)->", (long)obj->obj_id, obj->obj_size,
                ~~~                                 ^~~~~~~~~~~~~
                %lld

Issue 3: Unqualified calls to std::move

The file splaytree.hpp contained unqualified calls to std::move. The fix was simple and just required a change from invoking std::move instead of move. An example of the error is shown here:

libCacheSim/mrcProfiler/../dataStructure/splaytree.hpp:386:13: error: unqualified call to 'std::move' [-Werror,-Wunqualified-std-cast-call]
    root_ = move(n->right);
            ^
            std::

Issue 4: Use of deprecated sprintf function

The sprintf function is reported to be deprecated and insecure, so it is replaced by snprintf. An example of the error is shown here:

test/common.h:62:3: error: 'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Werror,-Wdeprecated-declarations]
  sprintf(data_path, "data/%s", data_name);

Issue 5: Unused variables

There are some unused variables that have been removed. An example of the error is shown here:

libCacheSim/traceAnalyzer/accessPattern.h:51:12: error: private field 'n_items' is not used [-Werror,-Wunused-private-field]
   int64_t n_items = 0;
           ^

Issue 6: C++ constructs used in C scope

The split_by_char function in bin/mrcProfiler/cli_parser.cpp returns an instance of type std::vector<std::string>. The problem here is that this function is declared within an extern "C" {...} block, so is treated as a C function type eventhough its return type is unsupported (only supported in C++). The fix here is to move the function outside of the extern block. An example of the error is shown here:

libCacheSim/bin/mrcProfiler/cli_parser.cpp:165:26: error: 'split_by_char' has C-linkage specified, but returns incomplete type 'std::vector<std::string>' (aka 'vector<basic_string<char>>') which could be incompatible with C [-Werror,-Wreturn-type-c-linkage]
std::vector<std::string> split_by_char(const char *input, const char &c) {
                         ^

Issue 7: Variadic argument list (...) with optional arguments is not handled correctly

The header file inclue/libCacheSim/macro.h defines some macros for assertions with optional arguments. The problem here is that this only works if __VA_ARGS__ is not empty. Otherwise, it leaves a dangling comma like CHECK_CONDITION(x, ==, NULL, FMT, ) and causes compilation errors on Clang (GCC silently ignores these). The solution is to wrap the comma in __VA_OPT__ like CHECK_CONDITION(x, ==, NULL, FMT __VA_OPT__(,) __VA_ARGS__), so the expansion only adds a comma when __VA_ARGS__ is not empty, else no dangling comma. An example of the error is shown here:

libCacheSim-fork/libCacheSim/profiler/simulator.c:207:3: error: expected expression
  ASSERT_NOT_NULL(gthread_pool, "cannot create thread pool in simulator\n");
  ^

libCacheSim/profiler/../include/libCacheSim/macro.h:92:3: note: expanded from macro 'ASSERT_NOT_NULL'
  CHECK_CONDITION(x, ==, NULL, FMT, __VA_ARGS__)
  ^

Issue 7: Unsupported linker options for macOS

There are three linker options used in CMakeLists.txt files across the project that are unsupported by Clang: export-dynamic, whole-archive, and no-whole-archive. Thus, logic has been added around these instances to ensure these options are only linked when run on Linux systems. An example of the error is shown here:

ld: unknown options: --export-dynamic --whole-archive --no-whole-archive 
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Issue 8: Specific GCC flags not recognised by Clang

The C file bin/cachesim/sim.c contains GCC-specific #pragma directives used to temporarily surpress the compiler warning -Wformat-truncation. The problem is that Clang does not recognize GCC-specific warnings. To fix this, preprocessor checks are applied to ensure the pragma is only applied for real GCC. An example of the error is shown here:

libCacheSim/bin/cachesim/sim.c:84:32: error: unknown warning group '-Wformat-truncation', ignored [-Werror,-Wunknown-warning-option]
#pragma GCC diagnostic ignored "-Wformat-truncation"

Issue 9: Linker issue with _create_cache function

The linker issue indicated that the definition of the _create_cache function could not be found. The problem here is that the C file plugin.c which contains the _create_cache function was never compiled and linked and caused an unresolved symbol error. To solve this, the cache/CMakeLists.txt was modified to include plugin.c in the source list for the cachelib target. An example of the error is shown here:

Undefined symbols for architecture arm64:
  "_create_cache", referenced from:
      mrcProfiler::MRCProfilerMINISIM::run() in libmrcProfilerLib.a[2](mrcProfiler.cpp.o)
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Issue 10: GNU-style variadic arguments not supported by Clang

GNU-style variadic arguments are used in the project, which Clang warns about by default. To resolve this by suppressing these warnings about zero-argument variadic macros, the compiler flag -Wno-gnu-zero-variadic-macro-arguments is added to both CMAKE_C_FLAGS and CMAKE_CXX_FLAGS in CMakeLists.txt. An example of the error is shown here:

libCacheSim/profiler/simulator.c:207:75: error: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]
  ASSERT_NOT_NULL(gthread_pool, "cannot create thread pool in simulator\n");

Issue 11: Library zstd not found

The zstd library was not found on macOS due to the path in which the library exists. Since brew is used to install packages on macOS, the path is different than the default path. To correctly handle this difference, the package manager pkg-config is used to link the zstd library correctly. An example of the error is shown here:

ld: library 'zstd' not found
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[laustam]

The SonarCloud failures are not my failures; these already existed but had not been triggered due to the CI configuration. They were now triggered due to the repository-wide reformat to conform to the clang-format style. Either way, the warnings shown on SonarCloud are not very useful, complaining about strlen being unsafe. Yeah, it can be unsafe, this is C code :)

[1a1a11a]

Thank you for the detailed summary! The code quality check was added recently, so it has not been run on most of the files.
Due to the amount of work needed, I was hoping that we would gradually converge: a few files would be cleaned each time.
You are amazing at leading the effort to clean up the code! Please let me know if we can help with anything. :)

[1a1a11a]

BTW, issue 2: we might want to use something like printf("%" PRIu64 "\n", t); or convert to unsigned long to avoid Linux reporting warnings.

[1a1a11a]

One more tip, if you want to break down the changes into smaller PRs, it might be easier to review them. :)

[sonarqubecloud]

Quality Gate failed

Failed conditions
6 Security Hotspots
8.6% Duplication on New Code (required ≤ 3%)
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[1a1a11a]

Are you still working on this? If we can resolve the build issue, we can enable macOS bulld in GitHub action #217

[1a1a11a]

I have fixed the build in #221

[laustam]

Sorry for the silence, I planned to break this down into smaller separate PRs as you had suggested previously. Issue 2 was fixed by PR 188

[1a1a11a]

Awesome! Note that the eviction tests fail on macOS, which still needs investigation.