Page MenuHomePhabricator

[Asan] Cleanup atomic usage in allocator
ClosedPublic

Authored by vitalybuka on Sep 1 2020, 1:41 AM.

Details

Summary

There are no know bugs related to this, still it may fix some latent ones.
Main concerns with preexisting code:

  1. Inconsistent atomic/non-atomic access to the same field.
  2. Assumption that bitfield chunk_state is always the first byte without even taking into account endianness.

Diff Detail

Event Timeline

vitalybuka created this revision.Sep 1 2020, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 1:41 AM
Herald added subscribers: Restricted Project, jfb. · View Herald Transcript
vitalybuka requested review of this revision.Sep 1 2020, 1:41 AM
vitalybuka added a reviewer: Restricted Project.Sep 1 2020, 5:21 AM
jfb added a comment.Sep 1 2020, 9:06 AM

Is this really NFC?

In D86917#2249793, @jfb wrote:

Is this really NFC?

I guess so, do you have particular concern?
I don't know particular cases where it was broken, but reading values some times as atomic and sometimes as not, plus unclear order of bit fields looks suspicions.

jfb added a comment.Sep 1 2020, 2:06 PM
In D86917#2249793, @jfb wrote:

Is this really NFC?

I guess so, do you have particular concern?
I don't know particular cases where it was broken, but reading values some times as atomic and sometimes as not, plus unclear order of bit fields looks suspicions.

It looks like a bunch of latent bugs being fixed, which is by definition changing functionality from "might be broken" to "correct" :-)
I'd remove the NFC tag.

vitalybuka retitled this revision from [NFC][Asan] Cleanup atomic usage in allocator to [Asan] Cleanup atomic usage in allocator.Sep 1 2020, 2:24 PM
vitalybuka edited the summary of this revision. (Show Details)

It looks like a bunch of latent bugs being fixed, which is by definition changing functionality from "might be broken" to "correct" :-)
I'd remove the NFC tag.

Done

vitalybuka edited the summary of this revision. (Show Details)Sep 1 2020, 2:24 PM
morehouse accepted this revision.Sep 2 2020, 3:58 PM
morehouse added a subscriber: morehouse.
morehouse added inline comments.
compiler-rt/lib/asan/asan_allocator.cpp
1083–1084

Remove this extra check.

This revision is now accepted and ready to land.Sep 2 2020, 3:58 PM
vitalybuka updated this revision to Diff 289809.Sep 3 2020, 2:21 PM
vitalybuka marked an inline comment as done.
vitalybuka edited the summary of this revision. (Show Details)

remove duplicate check

This revision was landed with ongoing or failed builds.Sep 3 2020, 2:22 PM
This revision was automatically updated to reflect the committed changes.

Hi Vitaly,

Did you make ChunkHeader bigger on purpose? Can you at least change the
comments, as it’s no longer the same size?

Thank you
Filipe

Did you make ChunkHeader bigger on purpose? Can you at least change the
comments, as it’s no longer the same size?

I'm also seeing that the build for windows is broken now, due to the COMPILER_CHECK(kChunkHeaderSize == 16); which is failing.

This can be reproduced in a smaller scale with this test snippet:

struct ChunkHeader {
  unsigned char chunk_state;
  unsigned int alloc_tid : 24;
};      

static_assert(sizeof(ChunkHeader) == 4, "unexpected size");

This fails when targeting Windows, as the Microsoft struct layout aligns the bitfield alloc_tid to a 4 byte boundary, following the preceding non-bitfield member.

Reverted this (and some follow-ups that caused conflicts during the revert) in rGdbf04aaade235a0d76c6ad549c091c9fd0ada0e8 to unbreak the Windows build.

compiler-rt/lib/asan/asan_allocator.cpp
73–74

This breaks Windows -- in the MS abi, only variables of the same type can be packed, and so the static_assert about the size fails.

...oh, mstorsjo alread reported this. Reverting, then :)

Hi Vitaly,

Did you make ChunkHeader bigger on purpose? Can you at least change the
comments, as it’s no longer the same size?

Sorry, it was not intentional.

vitalybuka reopened this revision.Sep 5 2020, 12:40 AM
This revision is now accepted and ready to land.Sep 5 2020, 12:40 AM
vitalybuka updated this revision to Diff 290117.Sep 5 2020, 7:04 PM

fix Windows

This revision was automatically updated to reflect the committed changes.