Skip to content

Fix warning-free build and tests - #1232

Open
takano32 wants to merge 2 commits into
avast:masterfrom
takano32:fix/build-test-warnings
Open

takano32 wants to merge 2 commits into
avast:masterfrom
takano32:fix/build-test-warnings

Conversation

@takano32

Copy link
Copy Markdown
Contributor

Summary

This change updates the build and test setup so the project builds cleanly without visible warnings or errors in the verified CMake build tree.

The main areas covered are:

  • Add compatibility patches for bundled third-party dependencies that fail or warn with newer CMake and compiler toolchains.
  • Suppress or route noisy third-party configure/build diagnostics through ExternalProject logs where appropriate.
  • Fix first-party compiler warnings in demangler, PDB parser, utility conversion code, and llvmir2hll tests.
  • Correct 10-byte floating-point handling on platforms where native long double is not the x87 80-bit representation.
  • Keep diagnostic assertions in tests meaningful without dereferencing null cast results while forming failure messages.

Details

Third-party build compatibility

  • Add ExternalProject timestamp handling to avoid CMake DOWNLOAD_EXTRACT_TIMESTAMP warnings.
  • Add CMake policy/version compatibility for bundled LLVM, Keystone, GoogleTest, and YaraMod sources.
  • Add patch steps for vendored projects that need small source/CMake updates with modern toolchains.
  • Route YARA configure/build output into logs to keep the top-level build output free from third-party autoconf/compiler noise.

First-party warning fixes

  • Change Borland demangler lookahead from char to int so EOF comparisons are valid.
  • Remove unused counters from PDB parser code.
  • Rework 80-bit-to-64-bit floating-point conversion to avoid GCC stringop-overflow diagnostics.
  • Fix llvmir2hll test assertion messages that previously dereferenced the null value being asserted.
  • Tighten long double platform detection to require the x87 80-bit format instead of assuming any >=10-byte long double is compatible.

Verification

Verified locally with:

Result:

  • Build exit code: 0
  • No matches for warning/error/failure diagnostics in the captured build log.

CTest was also invoked:

Result:

  • CTest exit code: 0
  • This build tree reports: No tests were found.

Because CTest did not enumerate tests in this build tree, all built test executables were run directly:

Result:

  • 13 test executables completed successfully.
  • Total: 7162 tests passed.
  • No failed tests.
  • No warning/error diagnostics found in the captured manual test log.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e9f1014c6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/utils/byte_value_storage.cpp Outdated
Comment on lines +539 to +541
bytes.resize(8);
double d = val;
memcpy(bytes.data(), &d, bytes.size());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep set10Byte writing a full 10-byte payload

With the new systemHasLongDouble() check, non-x87 platforms (e.g. long double as IEEE-128) now enter this branch and only write 8 bytes for set10Byte(). That regresses behavior for those targets because get10Byte() still reads 10 bytes, so a value written via set10Byte() can later fail to read back or include stale trailing bytes from previous data at address+8..9. This API should continue to serialize a complete 10-byte value (or explicitly pad/convert to 10 bytes) to stay consistent with the rest of the 10-byte read path.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in c9257f4.

You're right that set10Byte() wrote only 8 bytes on non-x87 platforms while get10Byte() always reads 10 bytes and decodes them as an 80-bit extended value, leaving the trailing 2 bytes stale. On top of that, the 8 written bytes were in native-double layout rather than the 80-bit extended layout the reader expects, so the value couldn't round-trip at all.

The fix adds double8ToDouble10() as the inverse of the existing double10ToDouble8() and uses it so the non-x87 branch always serializes a complete 10-byte extended value, keeping the write path symmetric with the read path.

Verified with a forward/inverse round-trip test: exact for all normal numbers, ±0, inf and nan. The double-subnormal range still flushes to zero, but that's the pre-existing behavior of double10ToDouble8()'s underflow handling on the read side, so symmetry holds.

set10Byte() wrote only an 8-byte native double on platforms that do not
use the x87 80-bit long double representation, while get10Byte() always
reads 10 bytes and decodes them as an 80-bit extended value. The two
trailing bytes were left stale and, worse, the 8 written bytes were in
native-double layout rather than the 80-bit extended layout the reader
expects, so values could not round-trip.

Add double8ToDouble10() as the inverse of double10ToDouble8() and use it
to serialize a complete 10-byte extended value, keeping the write path
symmetric with the read path. Round-trips are exact for all normal
numbers, zero, inf and nan; the double-subnormal range remains flushed
to zero by the existing double10ToDouble8() underflow handling.

Addresses Codex review feedback on avast#1232.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant