This is an archive of the discontinued LLVM Phabricator instance.

[gnu-objc] Make selector order deterministic.
ClosedPublic

Authored by theraven on Aug 10 2018, 6:40 AM.

Details

Summary

This probably fixes PR35277, though there may be other sources of
nondeterminism (this was the only case of iterating over a DenseMap).

It's difficult to provide a test case for this, because it shows up only
on systems with ASLR enabled.

Diff Detail

Repository
rC Clang

Event Timeline

theraven created this revision.Aug 10 2018, 6:40 AM

Does the error show up if you build llvm with -DLLVM_REVERSE_ITERATION:ON?

LLVM_REVERSE_ITERATION:BOOL
If enabled, all supported unordered llvm containers would be iterated in reverse order. This is useful for uncovering non-determinism caused by iteration of unordered containers.

got a build failure with this patch added onto 6.0.1

lib/CodeGen/CGObjCGNU.cpp
3547

compilation failed here:
../tools/clang/lib/CodeGen/CGObjCGNU.cpp:2444:11: error: 'sort' is not a member of 'llvm'

it suggested std::sort

got a build failure with this patch added onto 6.0.1

You may have to add:

#include "llvm/ADT/STLExtras.h"

You can't test that there's no non-determinism, but you can certainly test that we emit selectors in sorted order as opposed to the order in which they're used. I imagine a function with a bunch of @selector expressions should be good enough for that.

theraven added inline comments.Aug 11 2018, 4:15 AM
lib/CodeGen/CGObjCGNU.cpp
3547

I'm not sure llvm::sort was part of the 6.0 release, it was added in April and so should be in 7.0. I don't expect a patch against the 8 branch to apply cleanly to 6...

I got it compiled with std::sort on top of 6.0.1 and found that it indeed removes the indeterminism: When I compiled libobjc2-1.8.1/arc.m 25 times, I got the same md5sum every time - and the same result with and without ASLR.

theraven updated this revision to Diff 160312.Aug 13 2018, 2:17 AM
  • Add a test case.
rjmccall accepted this revision.Aug 13 2018, 12:03 PM

Thanks. I appreciate the fact that you spelled it all out in the test, too.

LGTM.

lib/CodeGen/CGObjCGNU.cpp
3547

array_pod_sort has been around forever if we need a variant of the patch for that branch.

This revision is now accepted and ready to land.Aug 13 2018, 12:03 PM
This revision was automatically updated to reflect the committed changes.