Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/22977 )

Change subject: [arm64] Fix SIGBUS errors for atomic operations
......................................................................


Patch Set 1:

(3 comments)

Thank you very much for the patch!

I followed up on one of the comment threads, suggesting to gather more details, 
if possible.

Do you see this issue happening only if compiling with CLANG18?  What if you 
compile Kudu and the test with GCC/G++ (say, GCC11)?

http://gerrit.cloudera.org:8080/#/c/22977/1/src/kudu/tablet/concurrent_btree.h
File src/kudu/tablet/concurrent_btree.h:

http://gerrit.cloudera.org:8080/#/c/22977/1/src/kudu/tablet/concurrent_btree.h@76
PS1, Line 76: static_assert(alignof(std::atomic<uint64_t>) == 8,
            :               "AtomicVersion requires 8-byte alignment");
I'm curious: how it might be less than 8 if sizeof(AtomicVersion) == 8?

>From the other side, wouldn't everything work as required if 
>alignof(std::atomic<uint64_t>) were 16 instead of 8?


http://gerrit.cloudera.org:8080/#/c/22977/1/src/kudu/tablet/concurrent_btree.h@78
PS1, Line 78: static_assert(sizeof(std::atomic<uint64_t>) == 8,
            :               "AtomicVersion must be 8 bytes");
I don't think this is necessary since there already static_assert at line 281 
(PS1):

  static_assert(sizeof(AtomicVersion) == 8, "must use 64bit version");

Also, there is a typedef at line 134:

  typedef std::atomic<std::uint64_t> AtomicVersion;


http://gerrit.cloudera.org:8080/#/c/22977/1/src/kudu/tablet/concurrent_btree.h@461
PS1, Line 461:   alignas(8) AtomicVersionValue versionPlaceholder_;
> Hi!
I agree with Zoltan: we need to fix this, for sure.  To be able to put together 
a proper fix, it's necessary to understand what's actually going on.

KeDeng, could you please run the test under gdb, catch the crash under the 
debugger, and clarify what variable isn't aligned as expected?

If you can see it's crashing in DEBUG build, I'd think that running a DEBUG 
binary under gdb could provide more details.

Thanks a lot!



--
To view, visit http://gerrit.cloudera.org:8080/22977
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f8ea46308f942ded97d93d27ae466fd13a31b53
Gerrit-Change-Number: 22977
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: KeDeng <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <[email protected]>
Gerrit-Reviewer: Zoltan Martonka <[email protected]>
Gerrit-Comment-Date: Fri, 13 Jun 2025 05:36:26 +0000
Gerrit-HasComments: Yes

Reply via email to