[PATCH] D50559: [gnu-objc] Make selector order deterministic.
This revision was automatically updated to reflect the committed changes. Closed by commit rC339668: [gnu-objc] Make selector order deterministic. (authored by theraven, committed by ). Changed prior to commit: https://reviews.llvm.org/D50559?vs=160312=160545#toc Repository: rC Clang https://reviews.llvm.org/D50559 Files: lib/CodeGen/CGObjCGNU.cpp test/CodeGenObjC/gnu-deterministic-selectors.m Index: test/CodeGenObjC/gnu-deterministic-selectors.m === --- test/CodeGenObjC/gnu-deterministic-selectors.m +++ test/CodeGenObjC/gnu-deterministic-selectors.m @@ -0,0 +1,16 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -fobjc-runtime=gnustep-1.5 %s -emit-llvm -o - | FileCheck %s +// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -fobjc-runtime=gcc %s -emit-llvm -o - | FileCheck %s + +// Check that these selectors are emitted in alphabetical order. +// The order doesn't actually matter, only that it doesn't vary across runs. +// Clang sorts them when targeting a GCC-like ABI to guarantee determinism. +// CHECK: @.objc_selector_list = internal global [6 x { i8*, i8* }] [{ i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namea, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_nameg, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namej, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namel, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namez, i64 0, i64 0), i8* null }, { i8*, i8* } zeroinitializer], align 8 + + +void f() { + SEL a = @selector(z); + SEL b = @selector(a); + SEL c = @selector(g); + SEL d = @selector(l); + SEL e = @selector(j); +} Index: lib/CodeGen/CGObjCGNU.cpp === --- lib/CodeGen/CGObjCGNU.cpp +++ lib/CodeGen/CGObjCGNU.cpp @@ -3541,12 +3541,16 @@ ConstantInitBuilder builder(CGM); auto selectors = builder.beginArray(selStructTy); auto = SelectorTable; // MSVC workaround -for (auto : table) { +std::vector allSelectors; +for (auto : table) + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); - std::string selNameStr = entry.first.getAsString(); +for (auto : allSelectors) { + std::string selNameStr = untypedSel.getAsString(); llvm::Constant *selName = ExportUniqueString(selNameStr, ".objc_sel_name"); - for (TypedSelector : entry.second) { + for (TypedSelector : table[untypedSel]) { llvm::Constant *selectorTypeEncoding = NULLPtr; if (!sel.first.empty()) selectorTypeEncoding = Index: test/CodeGenObjC/gnu-deterministic-selectors.m === --- test/CodeGenObjC/gnu-deterministic-selectors.m +++ test/CodeGenObjC/gnu-deterministic-selectors.m @@ -0,0 +1,16 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -fobjc-runtime=gnustep-1.5 %s -emit-llvm -o - | FileCheck %s +// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -fobjc-runtime=gcc %s -emit-llvm -o - | FileCheck %s + +// Check that these selectors are emitted in alphabetical order. +// The order doesn't actually matter, only that it doesn't vary across runs. +// Clang sorts them when targeting a GCC-like ABI to guarantee determinism. +// CHECK: @.objc_selector_list = internal global [6 x { i8*, i8* }] [{ i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namea, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_nameg, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namej, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namel, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namez, i64 0, i64 0), i8* null }, { i8*, i8* } zeroinitializer], align 8 + + +void f() { + SEL a = @selector(z); + SEL b = @selector(a); + SEL c = @selector(g); + SEL d = @selector(l); + SEL e = @selector(j); +} Index: lib/CodeGen/CGObjCGNU.cpp === --- lib/CodeGen/CGObjCGNU.cpp +++ lib/CodeGen/CGObjCGNU.cpp @@ -3541,12 +3541,16 @@ ConstantInitBuilder builder(CGM); auto selectors = builder.beginArray(selStructTy); auto = SelectorTable; // MSVC workaround -for (auto : table) { +std::vector allSelectors; +for (auto : table) + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); - std::string selNameStr = entry.first.getAsString(); +for (auto : allSelectors) { + std::string selNameStr =
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. I appreciate the fact that you spelled it all out in the test, too. LGTM. Comment at: lib/CodeGen/CGObjCGNU.cpp:3547 + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); theraven wrote: > bmwiedemann wrote: > > compilation failed here: > > ../tools/clang/lib/CodeGen/CGObjCGNU.cpp:2444:11: error: 'sort' is not a > > member of 'llvm' > > > > it suggested std::sort > 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... `array_pod_sort` has been around forever if we need a variant of the patch for that branch. Repository: rC Clang https://reviews.llvm.org/D50559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
theraven updated this revision to Diff 160312. theraven added a comment. - Add a test case. Repository: rC Clang https://reviews.llvm.org/D50559 Files: lib/CodeGen/CGObjCGNU.cpp test/CodeGenObjC/gnu-deterministic-selectors.m Index: test/CodeGenObjC/gnu-deterministic-selectors.m === --- /dev/null +++ test/CodeGenObjC/gnu-deterministic-selectors.m @@ -0,0 +1,16 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -fobjc-runtime=gnustep-1.5 %s -emit-llvm -o - | FileCheck %s +// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -fobjc-runtime=gcc %s -emit-llvm -o - | FileCheck %s + +// Check that these selectors are emitted in alphabetical order. +// The order doesn't actually matter, only that it doesn't vary across runs. +// Clang sorts them when targeting a GCC-like ABI to guarantee determinism. +// CHECK: @.objc_selector_list = internal global [6 x { i8*, i8* }] [{ i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namea, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_nameg, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namej, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namel, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namez, i64 0, i64 0), i8* null }, { i8*, i8* } zeroinitializer], align 8 + + +void f() { + SEL a = @selector(z); + SEL b = @selector(a); + SEL c = @selector(g); + SEL d = @selector(l); + SEL e = @selector(j); +} Index: lib/CodeGen/CGObjCGNU.cpp === --- lib/CodeGen/CGObjCGNU.cpp +++ lib/CodeGen/CGObjCGNU.cpp @@ -3541,12 +3541,16 @@ ConstantInitBuilder builder(CGM); auto selectors = builder.beginArray(selStructTy); auto = SelectorTable; // MSVC workaround -for (auto : table) { +std::vector allSelectors; +for (auto : table) + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); - std::string selNameStr = entry.first.getAsString(); +for (auto : allSelectors) { + std::string selNameStr = untypedSel.getAsString(); llvm::Constant *selName = ExportUniqueString(selNameStr, ".objc_sel_name"); - for (TypedSelector : entry.second) { + for (TypedSelector : table[untypedSel]) { llvm::Constant *selectorTypeEncoding = NULLPtr; if (!sel.first.empty()) selectorTypeEncoding = Index: test/CodeGenObjC/gnu-deterministic-selectors.m === --- /dev/null +++ test/CodeGenObjC/gnu-deterministic-selectors.m @@ -0,0 +1,16 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -fobjc-runtime=gnustep-1.5 %s -emit-llvm -o - | FileCheck %s +// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -fobjc-runtime=gcc %s -emit-llvm -o - | FileCheck %s + +// Check that these selectors are emitted in alphabetical order. +// The order doesn't actually matter, only that it doesn't vary across runs. +// Clang sorts them when targeting a GCC-like ABI to guarantee determinism. +// CHECK: @.objc_selector_list = internal global [6 x { i8*, i8* }] [{ i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namea, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_nameg, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namej, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namel, i64 0, i64 0), i8* null }, { i8*, i8* } { i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_sel_namez, i64 0, i64 0), i8* null }, { i8*, i8* } zeroinitializer], align 8 + + +void f() { + SEL a = @selector(z); + SEL b = @selector(a); + SEL c = @selector(g); + SEL d = @selector(l); + SEL e = @selector(j); +} Index: lib/CodeGen/CGObjCGNU.cpp === --- lib/CodeGen/CGObjCGNU.cpp +++ lib/CodeGen/CGObjCGNU.cpp @@ -3541,12 +3541,16 @@ ConstantInitBuilder builder(CGM); auto selectors = builder.beginArray(selStructTy); auto = SelectorTable; // MSVC workaround -for (auto : table) { +std::vector allSelectors; +for (auto : table) + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); - std::string selNameStr = entry.first.getAsString(); +for (auto : allSelectors) { + std::string selNameStr = untypedSel.getAsString(); llvm::Constant *selName = ExportUniqueString(selNameStr, ".objc_sel_name"); - for (TypedSelector : entry.second) { + for (TypedSelector : table[untypedSel]) { llvm::Constant
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
bmwiedemann added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D50559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
theraven added inline comments. Comment at: lib/CodeGen/CGObjCGNU.cpp:3547 + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); bmwiedemann wrote: > compilation failed here: > ../tools/clang/lib/CodeGen/CGObjCGNU.cpp:2444:11: error: 'sort' is not a > member of 'llvm' > > it suggested std::sort 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... Repository: rC Clang https://reviews.llvm.org/D50559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
rjmccall added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D50559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
mgrang added a comment. In https://reviews.llvm.org/D50559#1195787, @bmwiedemann wrote: > got a build failure with this patch added onto 6.0.1 You may have to add: #include "llvm/ADT/STLExtras.h" Repository: rC Clang https://reviews.llvm.org/D50559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
bmwiedemann added a comment. got a build failure with this patch added onto 6.0.1 Comment at: lib/CodeGen/CGObjCGNU.cpp:3547 + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); compilation failed here: ../tools/clang/lib/CodeGen/CGObjCGNU.cpp:2444:11: error: 'sort' is not a member of 'llvm' it suggested std::sort Repository: rC Clang https://reviews.llvm.org/D50559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
mgrang added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D50559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50559: [gnu-objc] Make selector order deterministic.
theraven created this revision. theraven added a reviewer: rjmccall. Herald added subscribers: cfe-commits, mgrang. 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. Repository: rC Clang https://reviews.llvm.org/D50559 Files: lib/CodeGen/CGObjCGNU.cpp Index: lib/CodeGen/CGObjCGNU.cpp === --- lib/CodeGen/CGObjCGNU.cpp +++ lib/CodeGen/CGObjCGNU.cpp @@ -3541,12 +3541,16 @@ ConstantInitBuilder builder(CGM); auto selectors = builder.beginArray(selStructTy); auto = SelectorTable; // MSVC workaround -for (auto : table) { +std::vector allSelectors; +for (auto : table) + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); - std::string selNameStr = entry.first.getAsString(); +for (auto : allSelectors) { + std::string selNameStr = untypedSel.getAsString(); llvm::Constant *selName = ExportUniqueString(selNameStr, ".objc_sel_name"); - for (TypedSelector : entry.second) { + for (TypedSelector : table[untypedSel]) { llvm::Constant *selectorTypeEncoding = NULLPtr; if (!sel.first.empty()) selectorTypeEncoding = Index: lib/CodeGen/CGObjCGNU.cpp === --- lib/CodeGen/CGObjCGNU.cpp +++ lib/CodeGen/CGObjCGNU.cpp @@ -3541,12 +3541,16 @@ ConstantInitBuilder builder(CGM); auto selectors = builder.beginArray(selStructTy); auto = SelectorTable; // MSVC workaround -for (auto : table) { +std::vector allSelectors; +for (auto : table) + allSelectors.push_back(entry.first); +llvm::sort(allSelectors.begin(), allSelectors.end()); - std::string selNameStr = entry.first.getAsString(); +for (auto : allSelectors) { + std::string selNameStr = untypedSel.getAsString(); llvm::Constant *selName = ExportUniqueString(selNameStr, ".objc_sel_name"); - for (TypedSelector : entry.second) { + for (TypedSelector : table[untypedSel]) { llvm::Constant *selectorTypeEncoding = NULLPtr; if (!sel.first.empty()) selectorTypeEncoding = ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits