This is an archive of the discontinued LLVM Phabricator instance.

[scudo][standalone] Introduce the chunk header
ClosedPublic

Authored by cryptoad on May 7 2019, 1:52 PM.

Event Timeline

cryptoad created this revision.May 7 2019, 1:52 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 7 2019, 1:52 PM
Herald added subscribers: Restricted Project, jfb, delcypher, mgorny. · View Herald Transcript
vitalybuka added inline comments.May 7 2019, 2:26 PM
lib/scudo/standalone/chunk.h
31

enum class ChunkState : u8 {

Available = 0,
Allocated = 1,
Quarantine = 2,

};

40

can you use your enums here instead of u64

43

why it uses u64 for 2bit fields?

59

does this have to be inline?

70

So you cant guaranty that it does not call computeChecksum before init() so you can avoid atomic operation?

cryptoad marked 5 inline comments as done.May 7 2019, 3:05 PM
cryptoad added inline comments.
lib/scudo/standalone/chunk.h
43

I remembered having issues where the end structure was not ending up on a u64, but it seems to work with enums.
I changed the type as well to be a class enum.

59

Yes. This does matter when compiled with SSE4.2 for example, the resulting compiled code end up being 2 successive crc32 instructions without any extraneous calls.

70

I am not sure I understand this, do you mean I should not use atomics?
The function should never be called prior to initialization (malloc will call the allocator's init() before servicing an allocation).

cryptoad updated this revision to Diff 198556.May 7 2019, 3:39 PM

Updated to use class enums, and put some structures that uses to
be in the scudo namespace into the scudo::Chunk one, as it
makes more sense.

vitalybuka added inline comments.May 7 2019, 3:46 PM
lib/scudo/standalone/chunk.h
70

If all threads will be blocked on init() and released after it's done, and global never changes after that then atomic is not needed

cryptoad marked 5 inline comments as done.May 7 2019, 3:47 PM
cryptoad added inline comments.
lib/scudo/standalone/chunk.h
70

This is indeed the case, I will remove the atomic use.

vitalybuka accepted this revision.May 7 2019, 3:53 PM
vitalybuka added inline comments.
lib/scudo/standalone/tests/chunk_test.cc
24

you can wrap everything into
namespace scudo::Chunk {
or just add using scudo::Chunk;

This revision is now accepted and ready to land.May 7 2019, 3:53 PM
cryptoad marked 2 inline comments as done.May 8 2019, 7:41 AM
cryptoad added inline comments.
lib/scudo/standalone/tests/chunk_test.cc
24

While I agree that this would simplify a bit the code, I think not using using or putting the tests in the namespace allows for more clarity as to where class and functions are located within the hierarchy of the project. I actually like it better and it forced me to rethink a bit some of the code organization when I was writing the tests.

cryptoad updated this revision to Diff 198659.May 8 2019, 7:41 AM

The HashAlgorithm global variable doesn't have to be atomic since
all alloc operations are stalled until init completes. Update things
accordingly (thanks Vitaly).

cryptoad updated this revision to Diff 198660.May 8 2019, 7:56 AM

Rename AllocTypeMask to OriginMask.

cryptoad updated this revision to Diff 198661.May 8 2019, 8:09 AM

Actually remove 2 unused masks.

cryptoad planned changes to this revision.May 8 2019, 8:52 AM

Working out some kinks on Fuchsia. Will update once it's ironed out.

cryptoad updated this revision to Diff 198687.May 8 2019, 9:37 AM

I had to backpedal on some of changes due to some errors uncovered
while testing on Fuchsia.

The main reason is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414
which complains that the number of bits used to store the enum class
is not enough (while it is, really). musl being compiled strictly on
Fuchsia, this ends up in an error. Keeping State and Origin as
enum class, except in the UnpackedHeader ended up requiring a
whole lot of static_cast.

In the end, to make it work on everything, I reverted back Origin
and State to standard enum, and added a comment explaining the
situation.

This revision is now accepted and ready to land.May 8 2019, 9:37 AM
cryptoad requested review of this revision.May 8 2019, 9:39 AM
vitalybuka accepted this revision.May 8 2019, 11:02 AM
This revision is now accepted and ready to land.May 8 2019, 11:02 AM
This revision was automatically updated to reflect the committed changes.