[PATCH] D121556: [randstruct] Add randomize structure layout support

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Apr 7 05:43:40 PDT 2022

aaron.ballman added a comment.

Recap: aside from the function merging behavior and the possible question about assertions in tests, I think this is ready to go.

Comment at: clang/lib/Sema/SemaDecl.cpp:27
 #include "clang/AST/NonTrivialTypeVisitor.h"
+#include "clang/AST/Randstruct.h"
 #include "clang/AST/StmtCXX.h"
void wrote:
> aaron.ballman wrote:
> > Is this include necessary?
> Yes. There's a call to `randstruct::randomizeStructureLayout` below.
Oh derp, I missed that. :-)

Comment at: clang/unittests/AST/RandstructTest.cpp:154-158
+#ifdef _WIN32
+  const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"};
+  const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"};
void wrote:
> aaron.ballman wrote:
> > Any idea what's gone wrong here? (Do we have a bug to file because these come out reversed? If so, can you add a FIXME comment here that we expect this test to change someday?)
> I think it's just a case where Windows' algorithm for `std::mt19937` is subtly different than the one for Linux. I'm not sure we should worry about it too much, to be honest. As long as it produces a deterministic output on the same platform we should be fine. I think it's expected that the same compiler/environment is used during all compilation steps. (I.e., one's not going to compile a module on Windows for a kernel build on Linux.)
Okay, that's a great reason for this to be left alone.

Comment at: clang/unittests/AST/RandstructTest.cpp:208-213
+// FIXME: Clang trips an assertion in the DiagnosticsEngine when the warning is
+// emitted while running under the test suite:
+// clang/lib/Frontend/TextDiagnosticPrinter.cpp:150: virtual void
+// clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level,
+// const clang::Diagnostic&): Assertion `TextDiag && "UnExpected diagnostic
+// outside source file processing"' failed.
void wrote:
> aaron.ballman wrote:
> > Is the assertion unrelated to the changes in your patch and you just happen to hit it with this test? (I get worried when tests trigger assertions.)
> To be honest, I haven't looked at these tests. I inherited them from the previous code base. I'll revisit these and probably delete them if they're not useful.
Okay, that sounds good to me.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list