Review comments.

Signed-off-by: fruffy <fruffy@nyu.edu>

Author: fruffy

lib/alloc_trace.cpp

@@ -75,9 +75,13 @@ std::ostream &operator<<(std::ostream &out, const AllocTrace &at) {
     std::sort(sorted.begin(), sorted.end(), [](auto &a, auto &b) { return a.first > b.first; });
 #if HAVE_LIBBACKTRACE
     if (!global_backtrace_state) {
-        // TODO: Do not use cstring here?
-        global_backtrace_state =
-            backtrace_create_state(cstring(getExecutablePath().c_str()).c_str(), 1, bt_error, &out);
+        // TODO: Do not use strdup here?
+        auto executablePath = getExecutablePath();
+        if (executablePath.empty()) {
+            executablePath = "<unknown_path>";
+        }
+        const char *executableChar = strdup(executablePath.c_str());
+        global_backtrace_state = backtrace_create_state(executableChar, 1, bt_error, &out);
     }
 #endif
 

lib/exename.cpp

@@ -19,22 +19,36 @@ limitations under the License.
 #include <array>
 #include <cstring>
 #include <filesystem>
+#include <system_error>
 
 namespace P4 {
 
-#if defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__NetBSD__)
-#include <unistd.h>
-
 #include <climits>
 #ifdef __APPLE__
+#include <unistd.h>
+
 #include <mach-o/dyld.h>
-#endif
 #elif defined(_WIN32)
 #include <windows.h>
+#else
+#include <unistd.h>
 #endif
 
 std::filesystem::path getExecutablePath(const std::filesystem::path &suggestedPath) {
-#if defined(__linux__) || defined(__FreeBSD__) || defined(__NetBSD__)
+#if defined(__APPLE__)
+    std::array<char, PATH_MAX> buffer{};
+    uint32_t size = static_cast<uint32_t>(buffer.size());
+    if (_NSGetExecutablePath(buffer.data(), &size) == 0) {
+        return buffer.data();
+    }
+#elif defined(_WIN32)
+    std::array<char, PATH_MAX> buffer{};
+    // TODO: Do we need to support this?
+    DWORD size = GetModuleFileNameA(nullptr, buffer.data(), static_cast<DWORD>(buffer.size()));
+    if (size > 0 && size < buffer.size()) {
+        return buffer.data();
+    }
+#else
     // Find the path of the executable.  We use a number of techniques that may fail or work on
     // different systems, and take the first working one we find.  Fallback to not overriding the
     // compiled-in installation path.
@@ -46,40 +60,20 @@ std::filesystem::path getExecutablePath(const std::filesystem::path &suggestedPa
     };
 
     for (const auto &path : paths) {
-        try {
-            // std::filesystem::canonical will throw on error if the path is invalid.
-            // It will also try to resolve symlinks.
-            return std::filesystem::canonical(path);
-        } catch (const std::filesystem::filesystem_error &) {
-            // Ignore and try the next path.
+        // std::filesystem::canonical will throw on error if the path is invalid.
+        // It will also try to resolve symlinks.
+        std::error_code errorCode;
+        auto canonicalPath = std::filesystem::canonical(path, errorCode);
+        // Return the path if no error was thrown.
+        if (!errorCode) {
+            return canonicalPath;
         }
     }
-#elif defined(__APPLE__)
-    std::array<char, PATH_MAX> buffer{};
-    uint32_t size = static_cast<uint32_t>(buffer.size());
-    if (_NSGetExecutablePath(buffer.data(), &size) == 0) {
-        return buffer.data();
-    } else
-#elif defined(_WIN32)
-    std::array<char, PATH_MAX> buffer{};
-    // TODO: Do we need to support this?
-    DWORD size = GetModuleFileNameA(nullptr, buffer.data(), static_cast<DWORD>(buffer.size()));
-    if (size > 0 && size < buffer.size()) {
-        return buffer.data();
-    } else
 #endif
-    if (auto *path = getenv("_")) {
-        return path;
-    }
     // If the above fails, try to convert suggestedPath to a path.
-    try {
-        // std::filesystem::canonical will throw on error if the path is invalid.
-        // It will also try to resolve symlinks.
-        return std::filesystem::canonical(suggestedPath);
-    } catch (const std::filesystem::filesystem_error &) {
-        // In the case of an error, return an empty path.
-        return {};
-    }
+    std::error_code errorCode;
+    auto canonicalPath = std::filesystem::canonical(suggestedPath, errorCode);
+    return errorCode ? std::filesystem::path() : canonicalPath;
 }
 
 const char *exename(const char *argv0) {

lib/exename.h

@@ -21,7 +21,8 @@ limitations under the License.
 namespace P4 {
 
 /// Return the full path to the binary being executed.
-/// @returns std::nullopt if unable to determine the path.
+/// @returns an empty path if unable to determine the path.
+/// TODO: With C++20 replace with std::expected?
 std::filesystem::path getExecutablePath(const std::filesystem::path &suggestedPath = "");
 
 /// Attempt to determine the executable name and return a static path to it.