From 394cd77bec89db829b3e8e84075e4dfbbf3eec2f Mon Sep 17 00:00:00 2001 From: "Markus F.X.J. Oberhumer" Date: Wed, 16 Aug 2023 01:06:52 +0200 Subject: [PATCH] all: cleanups --- .github/typos_config.toml | 2 +- .github/workflows/ci.yml | 4 ++-- .github/workflows/weekly-ci-llvm-mingw.yml | 8 ++++---- .github/workflows/weekly-ci-zigcc.yml | 4 ++-- src/check/dt_check.cpp | 2 +- src/packer.cpp | 7 ++----- src/packer.h | 6 +++--- src/packmast.cpp | 10 +++++----- src/packmast.h | 2 +- src/stub/src/include/header.S | 3 ++- src/ui.cpp | 2 +- src/util/membuffer.cpp | 11 ++++++----- src/util/util.cpp | 6 ++++++ src/util/util.h | 22 +++++++++++----------- src/util/xspan.cpp | 3 ++- 15 files changed, 49 insertions(+), 43 deletions(-) diff --git a/.github/typos_config.toml b/.github/typos_config.toml index ee754dde..b61ab10e 100644 --- a/.github/typos_config.toml +++ b/.github/typos_config.toml @@ -6,8 +6,8 @@ [files] extend-exclude = ["LICENSE", "misc/*podman*/*/packages.txt"] -[default.extend-identifiers] # misc variable names & symbols +[default.extend-identifiers] acc_spawnve = "acc_spawnve" ba = "ba" fo = "fo" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dbfe3142..a3a71717 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,8 +12,8 @@ env: CMAKE_REQUIRED_QUIET: OFF DEBIAN_FRONTEND: noninteractive UPX_CMAKE_BUILD_FLAGS: --verbose - # 2023-08-10 - ZIG_DIST_VERSION: 0.12.0-dev.47+0461a64a9 + # 2023-08-15 + ZIG_DIST_VERSION: 0.12.0-dev.89+913511557 jobs: job-rebuild-and-verify-stubs: diff --git a/.github/workflows/weekly-ci-llvm-mingw.yml b/.github/workflows/weekly-ci-llvm-mingw.yml index 962354ec..5fe22513 100644 --- a/.github/workflows/weekly-ci-llvm-mingw.yml +++ b/.github/workflows/weekly-ci-llvm-mingw.yml @@ -24,12 +24,12 @@ jobs: - name: llvm-mingw-20230614-ucrt llvm_version: 16.0.6 url: 'https://github.com/mstorsjo/llvm-mingw/releases/download/20230614/llvm-mingw-20230614-ucrt-ubuntu-20.04-x86_64.tar.xz' - - name: llvm-mingw-20230730-msvcrt + - name: llvm-mingw-20230811-msvcrt llvm_version: 17.0.0rc1 - url: 'https://github.com/mstorsjo/llvm-mingw/releases/download/20230730/llvm-mingw-20230730-msvcrt-ubuntu-20.04-x86_64.tar.xz' - - name: llvm-mingw-20230730-ucrt + url: 'https://github.com/mstorsjo/llvm-mingw/releases/download/20230811/llvm-mingw-20230811-msvcrt-ubuntu-20.04-x86_64.tar.xz' + - name: llvm-mingw-20230811-ucrt llvm_version: 17.0.0rc1 - url: 'https://github.com/mstorsjo/llvm-mingw/releases/download/20230730/llvm-mingw-20230730-ucrt-ubuntu-20.04-x86_64.tar.xz' + url: 'https://github.com/mstorsjo/llvm-mingw/releases/download/20230811/llvm-mingw-20230811-ucrt-ubuntu-20.04-x86_64.tar.xz' steps: - name: 'Check out code' uses: actions/checkout@v3 diff --git a/.github/workflows/weekly-ci-zigcc.yml b/.github/workflows/weekly-ci-zigcc.yml index 264dc8f4..90e0ea2d 100644 --- a/.github/workflows/weekly-ci-zigcc.yml +++ b/.github/workflows/weekly-ci-zigcc.yml @@ -8,8 +8,8 @@ on: env: CMAKE_REQUIRED_QUIET: OFF DEBIAN_FRONTEND: noninteractive - # 2023-08-10 - ZIG_DIST_VERSION: 0.12.0-dev.47+0461a64a9 + # 2023-08-15 + ZIG_DIST_VERSION: 0.12.0-dev.89+913511557 jobs: job-linux-zigcc: # uses cmake + make diff --git a/src/check/dt_check.cpp b/src/check/dt_check.cpp index ad5143d5..254fc3fa 100644 --- a/src/check/dt_check.cpp +++ b/src/check/dt_check.cpp @@ -577,7 +577,7 @@ void upx_compiler_sanity_check(void) noexcept { **************************************************************************/ TEST_CASE("assert_noexcept") { - // just to make sure that our own assert macros don't generate any warnings + // just to make sure that our own assert() macros don't generate any warnings byte dummy = 0; byte *ptr1 = &dummy; const byte *const ptr2 = &dummy; diff --git a/src/packer.cpp b/src/packer.cpp index b80cc2bb..bc8d24d6 100644 --- a/src/packer.cpp +++ b/src/packer.cpp @@ -44,16 +44,14 @@ PackerBase::PackerBase(InputFile *f) : fi(f), file_size(f ? f->st.st_size : 0) { Packer::Packer(InputFile *f) : PackerBase(f) { uip = new UiPacker(this); } Packer::~Packer() noexcept { - // owner owner_delete(uip); owner_delete(linker); assert_noexcept(linker == nullptr); - // references - bele = nullptr; } // for PackMaster void Packer::assertPacker() const { +#if DEBUG assert(getFormat() > 0); assert(getFormat() < 255); assert(getVersion() >= 11); @@ -76,7 +74,6 @@ void Packer::assertPacker() const { fprintf(stderr, "%s\n", getName()); assert(bele == format_bele); } -#if DEBUG Linker *l = newLinker(); assert(l != nullptr); if (bele != l->bele) @@ -509,7 +506,7 @@ unsigned Packer::getRandomId() const { // this is called directly after the constructor from class PackMaster void Packer::initPackHeader() { - mem_clear(&ph); + ph.reset(); ph.version = getVersion(); ph.format = getFormat(); ph.method = M_NONE; diff --git a/src/packer.h b/src/packer.h index 3f9f3de9..39ab32a3 100644 --- a/src/packer.h +++ b/src/packer.h @@ -59,11 +59,11 @@ public: // canPack() should throw a cantPackException eplaining why it cannot pack // a recognized format. - // canPack() can return -1 to stop early; see class PackMaster + // canPack() can return -1 to fail early; see class PackMaster virtual tribool canPack() = 0; // canUnpack() should throw a cantUnpackException eplaining why it cannot pack // a recognized format. - // canUnpack() can return -1 to stop early; see class PackMaster + // canUnpack() can return -1 to fail early; see class PackMaster virtual tribool canUnpack() = 0; // PackMaster entries @@ -82,7 +82,7 @@ protected: const upx_int64_t file_size; // must get set by constructor const upx_uint64_t file_size_u; // explicitly unsigned }; - PackHeader ph; // must be filled by canUnpack() + PackHeader ph; // must be filled by canUnpack(); also used by UiPacker }; /************************************************************************* diff --git a/src/packmast.cpp b/src/packmast.cpp index c303614b..b3acb13c 100644 --- a/src/packmast.cpp +++ b/src/packmast.cpp @@ -26,7 +26,7 @@ */ #include "headers.h" -#include +#include // std::unique_ptr #include "conf.h" #include "file.h" #include "packmast.h" @@ -102,7 +102,7 @@ static tribool try_can_pack(PackerBase *pb, void *user) may_throw { return true; // success } if (r.isOther()) // aka "-1" - return r; // canPack() says the format is recognized and we should stop early + return r; // canPack() says the format is recognized and we should fail early } catch (const IOException &) { // ignored } @@ -120,7 +120,7 @@ static tribool try_can_unpack(PackerBase *pb, void *user) may_throw { return true; // success } if (r.isOther()) // aka "-1" - return r; // canUnpack() says the format is recognized and we should stop early + return r; // canUnpack() says the format is recognized and we should fail early } catch (const IOException &) { // ignored } @@ -138,15 +138,15 @@ PackerBase *PackMaster::visitAllPackers(visit_func_t func, InputFile *f, const O ACC_BLOCK_BEGIN \ COMPILE_TIME_ASSERT(std::is_nothrow_destructible_v) \ auto pb = std::unique_ptr(new Klass(f)); \ - pb->assertPacker(); \ if (o->debug.debug_level) \ fprintf(stderr, "visitAllPackers: (ver=%d, fmt=%3d) %s\n", pb->getVersion(), \ pb->getFormat(), #Klass); \ + pb->assertPacker(); \ tribool r = func(pb.get(), user); \ if (r) \ return pb.release(); /* success */ \ if (r.isOther()) \ - return nullptr; /* stop early */ \ + return nullptr; /* stop and fail early */ \ ACC_BLOCK_END // NOTE: order of tries is important !!! diff --git a/src/packmast.h b/src/packmast.h index 16a61c25..1508b39c 100644 --- a/src/packmast.h +++ b/src/packmast.h @@ -52,7 +52,7 @@ public: private: OwningPointer(PackerBase) packer = nullptr; // owner - InputFile *fi = nullptr; // reference + InputFile *const fi; // reference, required static PackerBase *getPacker(InputFile *f); static PackerBase *getUnpacker(InputFile *f); diff --git a/src/stub/src/include/header.S b/src/stub/src/include/header.S index bf82962c..849c2e7d 100644 --- a/src/stub/src/include/header.S +++ b/src/stub/src/include/header.S @@ -31,10 +31,11 @@ section UPX1HEAD - .byte 85,80,88,33 // 0 UPX_MAGIC_LE32 + .byte 85,80,88,33 // 0 UPX_MAGIC_LE32 aka "UPX!" #if 0 .byte 161,216,208,213 // 4 UPX_MAGIC2_LE32 #else + // these will get set to actual values; initialized with some arbitrary numbers .byte 161 // 4 version .byte 216 // 5 format .byte 208 // 6 method diff --git a/src/ui.cpp b/src/ui.cpp index 9e69309f..ff471244 100644 --- a/src/ui.cpp +++ b/src/ui.cpp @@ -46,7 +46,7 @@ enum { M_CB_SCREEN // 2 line callback using screen }; -struct UiPacker::State { +struct UiPacker::State final { int mode; unsigned u_len; diff --git a/src/util/membuffer.cpp b/src/util/membuffer.cpp index 4715ad8f..f0c55ad7 100644 --- a/src/util/membuffer.cpp +++ b/src/util/membuffer.cpp @@ -62,7 +62,7 @@ static noinline void init_use_simple_mcheck() noexcept { static bool use_simple_mcheck() noexcept { static upx_std_once_flag init_done; upx_std_call_once(init_done, init_use_simple_mcheck); - // NOTE: clang-analyzer-unix.Malloc does not know that this flag is "constant" + // NOTE: clang-analyzer-unix.Malloc does not know that this flag is "constant"; see below return use_simple_mcheck_flag; } #else @@ -176,9 +176,10 @@ void MemBuffer::fill(unsigned off, unsigned len, int value) { // **************************************************************************/ -#define PTR_BITS(p) ((unsigned) ((upx_uintptr_t) (p) &0xffffffff)) -#define MAGIC1(p) ((PTR_BITS(p) ^ 0xfefdbeeb) | 1) -#define MAGIC2(p) ((PTR_BITS(p) ^ 0xfefdbeeb ^ 0x80024011) | 1) +// for use_simple_mcheck() +#define PTR_BITS32(p) ((unsigned) ((upx_uintptr_t) (p) &0xffffffff)) +#define MAGIC1(p) ((PTR_BITS32(p) ^ 0xfefdbeeb) | 1) +#define MAGIC2(p) ((PTR_BITS32(p) ^ 0xfefdbeeb ^ 0x88224411) | 1) void MemBuffer::checkState() const { if (!ptr) @@ -215,7 +216,7 @@ void MemBuffer::alloc(upx_uint64_t bytes) { // store magic constants to detect buffer overruns set_ne32(p - 8, size_in_bytes); set_ne32(p - 4, MAGIC1(p)); - set_ne32(p + size_in_bytes, MAGIC2(p)); + set_ne32(p + size_in_bytes + 0, MAGIC2(p)); set_ne32(p + size_in_bytes + 4, stats.global_alloc_counter); } ptr = (pointer) (void *) p; diff --git a/src/util/util.cpp b/src/util/util.cpp index 2843123f..ae94684e 100644 --- a/src/util/util.cpp +++ b/src/util/util.cpp @@ -787,6 +787,7 @@ TEST_CASE("get_ratio") { CHECK(get_ratio(2 * UPX_RSIZE_MAX, 1024ull * UPX_RSIZE_MAX) == 9999999); } +namespace { template struct TestTriBool { static void test(bool expect_true, int x) noexcept { @@ -831,9 +832,14 @@ struct TestTriBool { CHECK(a.isOther()); } }; +} // namespace TEST_CASE("TriBool") { + static_assert(!tribool(false)); + static_assert(tribool(true)); + static_assert(!tribool(tribool::Other)); TestTriBool::test(false, -1); + // TestTriBool >::test(false, -1); TestTriBool >::test(false, -1); // diff --git a/src/util/util.h b/src/util/util.h index ab96c04f..5c8c3852 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -209,24 +209,24 @@ public: static_assert(TOther != 0 && TOther != 1); enum value_type : underlying_type { False = 0, True = 1, Other = TOther }; // constructors - forceinline TriBool() noexcept = default; + forceinline constexpr TriBool() noexcept = default; forceinline ~TriBool() noexcept = default; constexpr TriBool(value_type x) noexcept : value(x) {} constexpr TriBool(promoted_type x) noexcept : value(x == 0 ? False : (x == 1 ? True : Other)) {} // access - value_type getValue() const noexcept { return value; } + constexpr value_type getValue() const noexcept { return value; } // checks for > 0, so TOther determines if Other is false (the default) or true - explicit operator bool() const noexcept { return value > False; } + explicit constexpr operator bool() const noexcept { return value > False; } // query; this is NOT the same as operator bool() - bool isStrictFalse() const noexcept { return value == False; } - bool isStrictTrue() const noexcept { return value == True; } - bool isStrictBool() const noexcept { return value == False || value == True; } - bool isOther() const noexcept { return value != False && value != True; } + constexpr bool isStrictFalse() const noexcept { return value == False; } + constexpr bool isStrictTrue() const noexcept { return value == True; } + constexpr bool isStrictBool() const noexcept { return value == False || value == True; } + constexpr bool isOther() const noexcept { return value != False && value != True; } // "other" can mean many things, depending on usage context, so provide some alternative names: - // forceinline bool isDefault() const noexcept { return isOther(); } // might be misleading - forceinline bool isIndeterminate() const noexcept { return isOther(); } - forceinline bool isUndecided() const noexcept { return isOther(); } - // forceinline bool isUnset() const noexcept { return isOther(); } // might be misleading + // constexpr bool isDefault() const noexcept { return isOther(); } // might be misleading + constexpr bool isIndeterminate() const noexcept { return isOther(); } + constexpr bool isUndecided() const noexcept { return isOther(); } + // constexpr bool isUnset() const noexcept { return isOther(); } // might be misleading constexpr bool operator==(TriBool other) const noexcept { return value == other.value; } constexpr bool operator==(value_type other) const noexcept { return value == other; } constexpr bool operator==(promoted_type other) const noexcept { return value == other; } diff --git a/src/util/xspan.cpp b/src/util/xspan.cpp index e5643a8a..29af435e 100644 --- a/src/util/xspan.cpp +++ b/src/util/xspan.cpp @@ -33,7 +33,7 @@ XSPAN_NAMESPACE_BEGIN // debugging stats struct XSpanStats { upx_std_atomic(size_t) check_range_counter; - // these normally will be zero, but doctest checks will populate them + // these usually will be zero, but internal doctest checks will populate them; see dt_xspan.cpp upx_std_atomic(size_t) fail_nullptr; upx_std_atomic(size_t) fail_nullbase; upx_std_atomic(size_t) fail_not_same_base; @@ -71,6 +71,7 @@ void xspan_fail_range_range() { } void xspan_check_range(const void *ptr, const void *base, ptrdiff_t size_in_bytes) { + // info: pointers are out of range deliberatly during internal doctest checks; see dt_xspan.cpp xspan_stats.check_range_counter += 1; if very_unlikely (ptr == nullptr) xspan_fail_range_nullptr();