In D128863#3620768, @tejohnson wrote:

During LTO a local promoted to a global gets a unique suffix based partially on the Module's ID.

The hash is actually a hash of the full module IR, computed when we write out the bitcode before the ThinLTO link, saved in the bitcode file, then read into a table indexed by module Id.

This has a problem, because the Module ID is not the same as the original Module ID.

Can you clarify what the different module IDs you are comparing here?

In D124836#3528521, @vvereschaka wrote:

got it. Yes, looks like it fixed. The test got passed during the last build: https://lab.llvm.org/buildbot/#/builders/104/builds/7812
Thank you.

Last update.

Fix up worklist loop.

Remove "goto" for a worklist.

Friendly ping.

Friendly ping. :)

Avoid warning about zero-sized arrays:

Remove the "{ 0 }" initialier since we don't need the "0" there.

Use a better method to create an empty entry.

Use better variable names and StringMap.

Update with better documentation and variable names.

In D124836#3507354, @nickdesaulniers wrote:

In D124836#3507268, @void wrote:

I'll split off the TableGen changes into a separate patch. It will supersede those changes here, so it shouldn't delay other reviews here. :-)

I'm referring to the changes to llvm/utils/TableGen/CallingConvEmitter.cpp and llvm/utils/TableGen/RegisterInfoEmitter.cpp. I would like to see those as a parent patch. I'm not sure what you're referring to, and it sounds like a child patch, not a parent patch.

Ugh!

I'll split off the TableGen changes into a separate patch. It will supersede those changes here, so it shouldn't delay other reviews here. :-)

Fix think-o use of HasSVE. Use --check-prefixes in the testcase.

• Used lists of argument registers generated from AArch64CallingConv.td.
• Add more tests for floating point and SVE.

@peterwaller-arm @sdesmalen Could you comment on what is considered the canonical way to zero Arm registers? Is mov x1, #0 the way or mov x1, xzr or some other way?

• Support SVE registers.
• Initial feature to gather argument registers from the *CallingConv.td files.

In D123763#3485836, @sbc100 wrote:

This new test has been failing on the emscripten builders.. seemingly ever since it landed:

https://ci.chromium.org/ui/p/emscripten-releases/builders/ci/linux-test-suites/b8815286583388131569/overview

In D124694#3486547, @void wrote:

In D124694#3485585, @aaron.ballman wrote:

struct t {
   int a, b, c, d, e;
} x = { .a = 2, 4, 5, 6 };

This situation seems like it should be an error, shouldn't it? The user specified one designated initializer (yay, that one is correct), but the rest are positional initializers and so there's no telling what they actually initialize due to field randomization.

That is diagnosed as an error. The issue is that after randomization, the a field is placed at the end of the structure. The initializer checker then sees the .a = 2 and says, "Ah! That's the one at the end of the structure. Any non-designated initializers afterwards will be excess ones," which is what happens. But that warning is completely mysterious to the end users who isn't told that they can't have a non-designated initializer on a randomized structure. Moving the diagnostic allows the correct warning to be emitted instead of the "excess elements" one.

In D124694#3485585, @aaron.ballman wrote:

struct t {
   int a, b, c, d, e;
} x = { .a = 2, 4, 5, 6 };

This situation seems like it should be an error, shouldn't it? The user specified one designated initializer (yay, that one is correct), but the rest are positional initializers and so there's no telling what they actually initialize due to field randomization.

Rebase with top-of-tree.

Fix test.

Add a few more tests and remove a dead check.

Gentle ping.

Maskray?

Fixed "std::vector" mishap

Correctly handle all types of Decls that can be found in a RecordDecl.

In D123958#3461714, @aaron.ballman wrote:

In D123958#3461020, @void wrote:

In D123958#3459205, @aaron.ballman wrote:

I think you'll need a more targeted approach than assuming the only kinds of declarations in a struct are field-like in C.

It seems that the issue you've got is with anonymous objects in a structure where the inner fields are available for lookup in the outer structure. One question I have is: what's the expectation for the user? There's two ways to look at this. 1) The anonymous object is a single field; that its members can be found in the outer object is not relevant. 2) The fields of the anonymous object should also be randomized. The same is true for any inner structure, not just anonymous ones.

I had assumed that any structure not marked for randomization would not be randomized. Based on that, I don't think inner structure objects (anonymous or otherwise) should automatically randomize their fields. WDYT?

We don't randomize inner structures unless they have the randomize_layout flag. That's always been the case, and this patch doesn't change that.

That's good, I misunderstood originally, so thanks!

The issue is that we were dropping the inner structures/unions because they aren't FieldDecls, but RecordDecls, with an IndirectFieldDecl if the inner structure is anonymous. The IndirectFieldDecl bits appear after the RecordDecl they're attached to, but doesn't seem like the ordering is important. The IndirectFieldDecl thing is there so that the anonymous fields are available in the outer structure.

Agreed that we need to fix that behavior, but I think it's pretty fragile to assume every declaration in a struct can be reordered. I mentioned static assert decls above as one obvious example, but also consider things like:

struct S {
  enum E {
    One,
  };

  enum E whatever;
} __attribute__((randomize_layout));

In D123958#3459205, @aaron.ballman wrote:

I think you'll need a more targeted approach than assuming the only kinds of declarations in a struct are field-like in C.

It seems that the issue you've got is with anonymous objects in a structure where the inner fields are available for lookup in the outer structure. One question I have is: what's the expectation for the user? There's two ways to look at this. 1) The anonymous object is a single field; that its members can be found in the outer object is not relevant. 2) The fields of the anonymous object should also be randomized. The same is true for any inner structure, not just anonymous ones.

I had assumed that any structure not marked for randomization would not be randomized. Based on that, I don't think inner structure objects (anonymous or otherwise) should automatically randomize their fields. WDYT?

See https://lab.llvm.org/buildbot#builders/186/builds/5509

Add tests for empty and single element structs.

Rebase.

Use "unique_ptr" when possible.

Updated with those changes.

Add some smoke tests.

Accept initialzers of the forms:

In D123544#3448046, @aaron.ballman wrote:

In D123544#3446814, @void wrote:

Let me work on that. But otherwise are you okay with this patch?

Yes, I'm okay with the direction of this patch. However, because this still automatically randomizes some structure layouts once the user passes the flag, I think the diagnostic above should perhaps land first unless there's some strong reason not to. (To be honest, I probably should have insisted on it in the first patch, but this one feels significantly more dangerous because it involves function pointers specifically and is more of an "automatic" feature than the first patch.)

In D123544#3446519, @xbolva00 wrote:

If we had error diagnostics when the user is about to shoot their foot off, I'd be more comfortable with the automatic hardening behavior.

This feature IMHO should be enabled with simple easy to understand flag, not silently and automatically do something which may break code or create surprises in debuggers.

In D123544#3446416, @aaron.ballman wrote:

However, I had forgotten that the base feature *requires* the user to pass a randomization seed via a flag in addition to requiring the attribute (thank you for bringing that back to my attention). Because this feature requires a feature flag to enable it, this behavior *is* a conforming extension (the user has to take an action to get the new behavior). That said, I'm still not convinced we want to do this automagically for users -- it's *really* easy for that flag to be set in a makefile somewhere and the user has no idea that their (non-designated) initialization is now a security vulnerability. If we had error diagnostics when the user is about to shoot their foot off, I'd be more comfortable with the automatic hardening behavior.

In D123544#3446285, @lebedev.ri wrote:

Does this not lead to non-deterministic/non-reproducible builds?

In D121556#3445124, @aaron.ballman wrote:

In D121556#3444837, @void wrote:

In D121556#3444434, @MaskRay wrote:

In D121556#3444260, @void wrote:

In D121556#3444221, @MaskRay wrote:

In D121556#3444131, @void wrote:

In D121556#3444021, @MaskRay wrote:

7aa8c38a9e190aea14116028c38b1d9f54cbb0b3 still uses std::shuffle, not incorporating the llvm::shuffle fixes I did.

You said it was still failing after the std::shuffle to llvm::shuffle change.

By saying it still failed, I meant there were other Windows vs non-Windows differences, not that std::shuffle=>llvm::shuffle was an unintended change.

So does it work or not? If I change it to llvm::shuffle, will the tests fail or will they pass regardless of the platform?

If you change std::shuffle to llvm::shuffle and add back tests like EXPECT_EQ(Expected, getFieldNamesFromRecord(RD));, the test will likely fail on Windows.
I recall that @aaron.ballman uses Windows and may help you find the differences.

It was also failing on MacOS. And it failed a different way on another Windows version. That's why I removed the test for the deterministic shuffle.

I tend to agree with your I think it's just a case where Windows' algorithm for std::mt19937 is subtly different than the one for Linux.

The only other option would be to add the EXPECT_* stuff on one platform (like Linux). I suppose that would be better than nothing.

Here's what has me confused... You're using std::mt19937 which is a very specific random number algorithm and you're giving it the same seed. I don't think we *should* be getting different behavior from the random number generator across platforms. The issue is the call to std::shuffle (see https://en.cppreference.com/w/cpp/algorithm/random_shuffle#Notes), so I think using llvm::shuffle will fix all the tests on all the platforms.