[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor] branch main updated: More fixes for compile-time warnings in equix and hashx



This is an automated email from the git hooks/post-receive script.

dgoulet pushed a commit to branch main
in repository tor.

The following commit(s) were added to refs/heads/main by this push:
     new cfbf74352f More fixes for compile-time warnings in equix and hashx
     new d5306e107f Merge branch 'tor-gitlab/mr/715'
cfbf74352f is described below

commit cfbf74352fece087f1080d87f939b226c3cf205b
Author: Micah Elizabeth Scott <beth@xxxxxxxxxxxxxx>
AuthorDate: Thu Jun 1 11:30:40 2023 -0700

    More fixes for compile-time warnings in equix and hashx
    
    This addresses issue #40800 and a couple other problems I noticed while
    trying to reproduce that one.
    
    The original issue is just a missing cast to void* on the args of
    __builtin___clear_cache(), and clang is picky about the implicit cast
    between what it considers to be char of different signedness. Original
    report is from MacOS but it's also reproducible on other clang targets.
    
    The cmake-based original build system for equix and hashx was a handy
    way to run tests, but it suffered from some warnings due to incorrect
    application of include_directories().
    
    And lastly, there were some return codes from hashx_exec() that get
    ignored on equix when asserts are disabled. It bugged me too much to
    just silence this with a (void) cast, since even though this is in the
    realm of low-likelyhood programming errors and not true runtime errors, I
    don't want to make it easy for the hashx_exec() wrappers to return
    values that are dangerously wrong if an error is ignored. I made sure
    that even if asserts are disabled, we return values that will cause the
    solver and verifier to both fail to validate a potential solution.
    
    Signed-off-by: Micah Elizabeth Scott <beth@xxxxxxxxxxxxxx>
---
 changes/ticket40800                    |  3 +++
 src/ext/equix/CMakeLists.txt           | 18 +++++-------------
 src/ext/equix/hashx/CMakeLists.txt     |  8 ++------
 src/ext/equix/hashx/src/compiler_a64.c |  2 +-
 src/ext/equix/src/equix.c              |  7 +++++--
 src/ext/equix/src/solver.c             | 15 +++++++++++----
 6 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/changes/ticket40800 b/changes/ticket40800
new file mode 100644
index 0000000000..e2ebc80ee8
--- /dev/null
+++ b/changes/ticket40800
@@ -0,0 +1,3 @@
+  o Minor feature (hs):
+    - Fix compiler warnings in equix and hashx when building with clang.
+      Closes ticket 40800.
diff --git a/src/ext/equix/CMakeLists.txt b/src/ext/equix/CMakeLists.txt
index 032989d804..3c4606566f 100644
--- a/src/ext/equix/CMakeLists.txt
+++ b/src/ext/equix/CMakeLists.txt
@@ -24,13 +24,14 @@ if(NOT CMAKE_BUILD_TYPE)
   message(STATUS "Setting default build type: ${CMAKE_BUILD_TYPE}")
 endif()
 
-add_library(equix SHARED ${equix_sources})
-set_property(TARGET equix PROPERTY POSITION_INDEPENDENT_CODE ON)
-set_property(TARGET equix PROPERTY PUBLIC_HEADER include/equix.h)
-include_directories(equix
+include_directories(
   include/
   hashx/include/
   hashx/src/)
+
+add_library(equix SHARED ${equix_sources})
+set_property(TARGET equix PROPERTY POSITION_INDEPENDENT_CODE ON)
+set_property(TARGET equix PROPERTY PUBLIC_HEADER include/equix.h)
 target_compile_definitions(equix PRIVATE HASHX_STATIC)
 target_compile_definitions(equix PRIVATE EQUIX_SHARED)
 target_link_libraries(equix
@@ -43,10 +44,6 @@ set_property(TARGET equix_static PROPERTY POSITION_INDEPENDENT_CODE ON)
 set_target_properties(equix_static PROPERTIES OUTPUT_NAME equix)
 target_compile_definitions(equix_static PRIVATE HASHX_STATIC)
 target_compile_definitions(equix_static PRIVATE EQUIX_STATIC)
-include_directories(equix_static
-  include/
-  hashx/include/
-  hashx/src/)
 target_link_libraries(equix_static
   PRIVATE hashx_static)
 
@@ -58,8 +55,6 @@ install(TARGETS equix equix_static
 
 add_executable(equix-tests
   src/tests.c)
-include_directories(equix-tests
-  include/)
 target_compile_definitions(equix-tests PRIVATE EQUIX_STATIC)
 target_link_libraries(equix-tests
   PRIVATE equix_static)
@@ -73,9 +68,6 @@ add_executable(equix-bench
   src/bench.c
   hashx/src/hashx_thread.c
   hashx/src/hashx_time.c)
-include_directories(equix-bench
-  include/
-  hashx/src/)
 target_compile_definitions(equix-bench PRIVATE EQUIX_STATIC)
 target_link_libraries(equix-bench
   PRIVATE equix_static
diff --git a/src/ext/equix/hashx/CMakeLists.txt b/src/ext/equix/hashx/CMakeLists.txt
index 742a5a8523..1e8fe2fd6c 100644
--- a/src/ext/equix/hashx/CMakeLists.txt
+++ b/src/ext/equix/hashx/CMakeLists.txt
@@ -55,11 +55,11 @@ if(HASHX_SALT)
   endif()
 endif()
 
+include_directories(include/)
+
 add_library(hashx SHARED ${hashx_sources})
 set_property(TARGET hashx PROPERTY POSITION_INDEPENDENT_CODE ON)
 set_property(TARGET hashx PROPERTY PUBLIC_HEADER include/hashx.h)
-include_directories(hashx
-  include/)
 target_compile_definitions(hashx PRIVATE HASHX_SHARED)
 set_target_properties(hashx PROPERTIES VERSION ${HASHX_VERSION_STR}
                                        SOVERSION ${HASHX_VERSION})
@@ -76,8 +76,6 @@ install(TARGETS hashx hashx_static
 
 add_executable(hashx-tests
   src/tests.c)
-include_directories(hashx-tests
-  include/)
 target_compile_definitions(hashx-tests PRIVATE HASHX_STATIC)
 target_link_libraries(hashx-tests
   PRIVATE hashx_static)
@@ -91,8 +89,6 @@ add_executable(hashx-bench
   src/bench.c
   src/hashx_thread.c
   src/hashx_time.c)
-include_directories(hashx-bench
-  include/)
 target_compile_definitions(hashx-bench PRIVATE HASHX_STATIC)
 target_link_libraries(hashx-bench
   PRIVATE hashx_static
diff --git a/src/ext/equix/hashx/src/compiler_a64.c b/src/ext/equix/hashx/src/compiler_a64.c
index 94635ad1b7..1b7081f537 100644
--- a/src/ext/equix/hashx/src/compiler_a64.c
+++ b/src/ext/equix/hashx/src/compiler_a64.c
@@ -149,7 +149,7 @@ bool hashx_compile_a64(const hashx_program* program, uint8_t* code) {
 	if (!hashx_vm_rx(code, COMP_CODE_SIZE))
 		return false;
 #ifdef __GNUC__
-	__builtin___clear_cache(code, pos);
+	__builtin___clear_cache((void*)code, (void*)pos);
 #endif
 	return true;
 }
diff --git a/src/ext/equix/src/equix.c b/src/ext/equix/src/equix.c
index a254261509..5511201695 100644
--- a/src/ext/equix/src/equix.c
+++ b/src/ext/equix/src/equix.c
@@ -28,8 +28,11 @@ static uint64_t sum_pair(hashx_ctx* hash_func, equix_idx left, equix_idx right)
 	uint8_t hash_right[HASHX_SIZE];
 	hashx_result r_left = hashx_exec(hash_func, left, hash_left);
 	hashx_result r_right = hashx_exec(hash_func, right, hash_right);
-	assert(r_left == HASHX_OK && r_right == HASHX_OK);
-	return load64(hash_left) + load64(hash_right);
+	if (r_left == HASHX_OK && r_right == HASHX_OK) {
+		return load64(hash_left) + load64(hash_right);
+	}
+	assert(false);
+	return ~(uint64_t)0;
 }
 
 static equix_result verify_internal(hashx_ctx* hash_func, const equix_solution* solution) {
diff --git a/src/ext/equix/src/solver.c b/src/ext/equix/src/solver.c
index 1beda06c74..618c6e1130 100644
--- a/src/ext/equix/src/solver.c
+++ b/src/ext/equix/src/solver.c
@@ -47,11 +47,16 @@ typedef stage1_idx_item s1_idx;
 typedef stage2_idx_item s2_idx;
 typedef stage3_idx_item s3_idx;
 
-static FORCE_INLINE uint64_t hash_value(hashx_ctx* hash_func, equix_idx index) {
+static FORCE_INLINE bool hash_value(hashx_ctx* hash_func, equix_idx index, uint64_t *value_out) {
 	char hash[HASHX_SIZE];
 	hashx_result result = hashx_exec(hash_func, index, hash);
-	assert(result == HASHX_OK);
-	return load64(hash);
+	if (result == HASHX_OK) {
+		*value_out = load64(hash);
+		return true;
+	} else {
+		assert(false);
+		return false;
+	}
 }
 
 static void build_solution_stage1(equix_idx* output, solver_heap* heap, s2_idx root) {
@@ -97,7 +102,9 @@ static void build_solution(equix_solution* solution, solver_heap* heap, s3_idx l
 static void solve_stage0(hashx_ctx* hash_func, solver_heap* heap) {
 	CLEAR(heap->stage1_indices.counts);
 	for (u32 i = 0; i < INDEX_SPACE; ++i) {
-		uint64_t value = hash_value(hash_func, i);
+		uint64_t value;
+		if (!hash_value(hash_func, i, &value))
+			break;
 		u32 bucket_idx = value % NUM_COARSE_BUCKETS;
 		u32 item_idx = STAGE1_SIZE(bucket_idx);
 		if (item_idx >= COARSE_BUCKET_ITEMS)

-- 
To stop receiving notification emails like this one, please contact
the administrator of this repository.
_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits