chenBright commented on code in PR #3696:
URL: https://github.com/apache/brpc/pull/3696#discussion_r4441967141
##########
src/brpc/mongo_head.h:
##########
@@ -62,12 +62,12 @@ struct mongo_head_t {
int32_t op_code; // request type, see MongoOpCode.
void make_host_endian() {
- if (!ARCH_CPU_LITTLE_ENDIAN) {
- message_length = butil::ByteSwap((uint32_t)message_length);
- request_id = butil::ByteSwap((uint32_t)request_id);
- response_to = butil::ByteSwap((uint32_t)response_to);
- op_code = butil::ByteSwap((uint32_t)op_code);
- }
+#if !defined(ARCH_CPU_LITTLE_ENDIAN)
+ message_length = butil::ByteSwap((uint32_t)message_length);
+ request_id = butil::ByteSwap((uint32_t)request_id);
+ response_to = butil::ByteSwap((uint32_t)response_to);
+ op_code = butil::ByteSwap((uint32_t)op_code);
Review Comment:
On the big-endian ELFv2 platform added in this change, converting only the
local header here is still insufficient to handle valid Mongo requests:
`ParseMongoMessage()` subsequently stores the original little-endian header in
`msg->meta`, and `ProcessMongoRequest()` reads it directly in host byte order.
For example, the bytes `d4 07 00 00` for `op_code=2004` are read as
`0xd4070000` and rejected by `MongoOp_IsValid()`, so the application callback
is never invoked. Please ensure consistent conversion to host byte order before
dispatch, explicitly encode response fields as little-endian, and add
big-endian end-to-end tests using fixed little-endian bytes.
##########
src/butil/atomicops_internals_gcc.h:
##########
@@ -99,6 +93,82 @@ inline Atomic32 Release_Load(volatile const Atomic32* ptr) {
return *ptr;
}
+#ifdef ARCH_CPU_64_BITS
+
+inline Atomic64 NoBarrier_CompareAndSwap(volatile Atomic64* ptr,
+ Atomic64 old_value,
+ Atomic64 new_value) {
+ return __sync_val_compare_and_swap(ptr, old_value, new_value);
+}
+
+inline Atomic64 NoBarrier_AtomicExchange(volatile Atomic64* ptr,
+ Atomic64 new_value) {
+ Atomic64 old_value;
+ do {
+ old_value = *ptr;
+ } while (!__sync_bool_compare_and_swap(ptr, old_value, new_value));
+ return old_value;
+}
+
+inline Atomic64 Barrier_AtomicIncrement(volatile Atomic64* ptr,
+ Atomic64 increment) {
+ for (;;) {
+ Atomic64 old_value = *ptr;
+ Atomic64 new_value = old_value + increment;
+ if (__sync_bool_compare_and_swap(ptr, old_value, new_value)) {
Review Comment:
`Atomic64` is a signed `intptr_t` on this target, so when `*ptr ==
INTPTR_MAX` and `increment == 1`, the addition here causes signed overflow
before reaching the CAS; CAS atomicity does not give this expression wraparound
semantics. The existing `AtomicOpsTest.IncrementBounds` explicitly requires
`AtomicWord` to wrap between its maximum and minimum values. Consider using
`__sync_add_and_fetch` directly, or preserving the result's bit pattern through
unsigned arithmetic, and verify the boundary cases and overflow detection
results on PPC64.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
