ArenaTest.BlockSizeSmallerThanAllocation fails on 32 bit alpine linux

[ncopa]

What version of protobuf and what language are you using?
Version: protobuf-3.15.7
Language: C++

What operating system (Linux, Windows, ...) and version?
Alpine linux edge (rolling release)

What runtime / compiler are you using (e.g., python version or gcc version)
gcc (Alpine 10.2.1_git20210328) 10.2.1 20210328
Python 3.9.4

What did you do?
Steps to reproduce the behavior:

1. CXXFLAGS="$CXXFLAGS -fno-delete-null-pointer-checks -Wno-error" ./configure --prefix=/usr ...
2. make -j24
3. cd src
4. make -j24 protobuf-test && ./protobuf-test --gtest_filter=ArenaTest.BlockSizeSmallerThanAllocation

What did you expect to see
test passed

What did you see instead?

Running main() from gmock_main.cc
Note: Google Test filter = ArenaTest.BlockSizeSmallerThanAllocation
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from ArenaTest
[ RUN      ] ArenaTest.BlockSizeSmallerThanAllocation
google/protobuf/arena_unittest.cc:1260: Failure
Expected equality of these values:
  8
  arena.SpaceUsed()
    Which is: 12
google/protobuf/arena_unittest.cc:1264: Failure
Expected equality of these values:
  16
  arena.SpaceUsed()
    Which is: 20
google/protobuf/arena_unittest.cc:1260: Failure
Expected equality of these values:
  8
  arena.SpaceUsed()
    Which is: 12
google/protobuf/arena_unittest.cc:1264: Failure
Expected equality of these values:
  16
  arena.SpaceUsed()
    Which is: 20
google/protobuf/arena_unittest.cc:1260: Failure
Expected equality of these values:
  8
  arena.SpaceUsed()
    Which is: 12
google/protobuf/arena_unittest.cc:1264: Failure
Expected equality of these values:
  16
  arena.SpaceUsed()
    Which is: 20
google/protobuf/arena_unittest.cc:1260: Failure
Expected equality of these values:
  8
  arena.SpaceUsed()
    Which is: 12
google/protobuf/arena_unittest.cc:1264: Failure
Expected equality of these values:
  16
  arena.SpaceUsed()
    Which is: 20
google/protobuf/arena_unittest.cc:1260: Failure
Expected equality of these values:
  8
  arena.SpaceUsed()
    Which is: 12
google/protobuf/arena_unittest.cc:1264: Failure
Expected equality of these values:
  16
  arena.SpaceUsed()
    Which is: 20
google/protobuf/arena_unittest.cc:1260: Failure
Expected equality of these values:
  8
  arena.SpaceUsed()
    Which is: 12
google/protobuf/arena_unittest.cc:1264: Failure
Expected equality of these values:
  16
  arena.SpaceUsed()
    Which is: 20
google/protobuf/arena_unittest.cc:1260: Failure
Expected equality of these values:
  8
  arena.SpaceUsed()
    Which is: 12
google/protobuf/arena_unittest.cc:1264: Failure
Expected equality of these values:
  16
  arena.SpaceUsed()
    Which is: 20
google/protobuf/arena_unittest.cc:1260: Failure
Expected equality of these values:
  8
  arena.SpaceUsed()
    Which is: 12
google/protobuf/arena_unittest.cc:1264: Failure
Expected equality of these values:
  16
  arena.SpaceUsed()
    Which is: 20
google/protobuf/arena_unittest.cc:1260: Failure
Expected equality of these values:
  8
  arena.SpaceUsed()
    Which is: 12
google/protobuf/arena_unittest.cc:1264: Failure
Expected equality of these values:
  16
  arena.SpaceUsed()
    Which is: 20
[  FAILED  ] ArenaTest.BlockSizeSmallerThanAllocation (0 ms)
[----------] 1 test from ArenaTest (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] ArenaTest.BlockSizeSmallerThanAllocation

 1 FAILED TEST

Anything else we should know about your project / environment
This happens on both 32 bit x86 and armv7.

musl libc is used.

Probably same underlying issue as #8367

[ncopa]

This change makes the test pass, which still tests if the allocation is bigger than space used:

diff --git a/src/google/protobuf/arena_unittest.cc b/src/google/protobuf/arena_unittest.cc
index 7e90164..cf633e3 100644
--- a/src/google/protobuf/arena_unittest.cc
+++ b/src/google/protobuf/arena_unittest.cc
@@ -1256,12 +1256,10 @@ TEST(ArenaTest, BlockSizeSmallerThanAllocation) {
     Arena arena(opt);
 
     *Arena::Create<int64>(&arena) = 42;
-    EXPECT_GE(arena.SpaceAllocated(), 8);
-    EXPECT_EQ(8, arena.SpaceUsed());
+    EXPECT_GE(arena.SpaceAllocated(), arena.SpaceUsed());
 
     *Arena::Create<int64>(&arena) = 42;
-    EXPECT_GE(arena.SpaceAllocated(), 16);
-    EXPECT_EQ(16, arena.SpaceUsed());
+    EXPECT_GE(arena.SpaceAllocated(), arena.SpaceUsed());
   }
 }
 

[ncopa]

I found the problem:

protobuf/src/google/protobuf/arena.cc

Line 424 in a001250

return space_used - (AllocPolicy() ? sizeof(AllocationPolicy) : 0);

The problem is that the AllocationPolicy struct has a couple pointers, so the size is different on 32 bit and 64 bit platforms. Since the size of the struct is subtracted the reported SpaceUsed gets higher on 32 bit platforms.

I think the test is broken in this case because it checks against a fixed size, regardless of platform.

[fowles]

seems like we should be able to use sizeof(void*) or similar. Do you mind taking a crack at it?

[ncopa]

This seems to make this specific test pass on both 64bit and 32bit:

diff --git a/src/google/protobuf/arena_unittest.cc b/src/google/protobuf/arena_unittest.cc
index 7e9016429..178ad13fd 100644
--- a/src/google/protobuf/arena_unittest.cc
+++ b/src/google/protobuf/arena_unittest.cc
@@ -1257,11 +1257,11 @@ TEST(ArenaTest, BlockSizeSmallerThanAllocation) {
 
     *Arena::Create<int64>(&arena) = 42;
     EXPECT_GE(arena.SpaceAllocated(), 8);
-    EXPECT_EQ(8, arena.SpaceUsed());
+    EXPECT_EQ(16 - sizeof(void*), arena.SpaceUsed());
 
     *Arena::Create<int64>(&arena) = 42;
     EXPECT_GE(arena.SpaceAllocated(), 16);
-    EXPECT_EQ(16, arena.SpaceUsed());
+    EXPECT_EQ(24 - sizeof(void*), arena.SpaceUsed());
   }
 }
 

but, I'm not really familiar with the code and I have no idea how the SpaceUsed calculation is supposed to work.

In the SpaceAllocated_and_Used test (which also fails) it seems that there are a requirement that SpaceUsed must be aligned to 8 bytes, so there might be some padding needed in the structs on 32 bit platforms? or should it be aligned to 4 bytes on 32 bit platforms? I have really no idea.

[ncopa]

The align8 thingy was introduced in 885b612, but there are no explanation in commit message why it needs to be aligned that way.

The sizeof(AllocPolicy) thingy - which likely broke it - seems to be introduced in commit 5c028d6, without any good explanation in commit message.

I'm giving up. I'll let people who know this code deal with it.

[fowles]

that is fair, thanks for taking a look.

[Apteryks]

I also have this test failing along ArenaTest.SpaceAllocated_and_Used on i686-linux (32 bits as well).

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.