In D43700#1019506, @zturner wrote:

@rnk by "in an MSVC environment" do you mean "when -fms-compatibility is present"?

In D43700#1019505, @aganea wrote:

Thanks for fixing this @zturner. I simply want to back-up what @probinson says above - most of the games we do at Ubisoft are currently using a different compilation toolchains for each platform (we ship at least 4-5 platforms for each top game). It can be clang or it can be MSVC; and most of the time it's different versions of clang. Our long-term goal is to preferably have only one toolchain for all our platforms & all our targets - that means one set of common g++ like flags, including Windows.

In D43700#1018087, @zturner wrote:

In D43700#1018042, @colden wrote:

Seems good to me! I'll give it a test on my end.

One alternate implementation idea though, what if you defaulted EmitCodeView to the hasArg check instead of false, then removed the else *EmitCodeView = false; block on line 4999?

That would actually change the behavior of the cl driver, which I kind of don't want to do since it's not necessary. It would whitelist an additional clang option to be recognized by the cl driver. Specifically, you could then get debug info via the cl driver without specifying /Z7 or /Zi. It makes the possibilities more confusing, and someone will invariably try to do it and screw something up. We already whitelist some dash options so that the cl driver will recognize them, but it's on a case by case basis and only when there's a strong need for it.

Seems good to me! I'll give it a test on my end.

In D42496#988265, @smeenai wrote:

In D42496#986897, @colden wrote:

In D42496#986872, @smeenai wrote:

For future reference, you should upload diffs with more context (-U9999, or just use arcanist). This is trivial so it doesn't matter that much. It's also a good idea to add some reviewers explicitly, otherwise it's easy for things to slip through on the mailing list.

I did use arcanist, is there a special config option I have to set or something? I'm on Windows too, so maybe that's it? I'll dig around the internet to see if I can fix it for next time.

arc diff should submit a diff with full context by default, but arc in general isn't entirely sane on Windows, so it wouldn't surprise me if it's doing something silly.

In D42496#986872, @smeenai wrote:

LGTM.

Thanks for reviewing this for me!

Confirmed, this fixes my issue! Thanks!

Hey guys, is there anything holding this back? If not, I'd love it if one of you could submit it for me.

Rebased on top of the timer change

Yay, thanks! This is one of the last bugs that's blocking me. At some point I'll have to figure out how to get these changes merged to 6.0...

Got as close as I could to @rnk's suggested error format, while still using toString(theError).

Changes requested by @rnk. Warning is now handled by addObjFile().

Works for me! Update incoming.

Ok, that makes sense.

Bump, could someone submit this for me?

Thanks for the quick reviews!

Turns out you can do the easier thing

Switch error message to use decimal format from hex.

Thank you Zach!

Ping, could someone please submit this for me?

If you wouldn't mind committing this for me, I'd really appreciate it.

nth time's the charm!

Updated with my newer, better understanding of the file layout

Now that I actually understand how the intervals work, I reworked a lot of what I had written. I think it's much clearer now, but I'm sure there's more nits to be had :)

In D41825#970042, @zturner wrote:

In D41825#970040, @colden wrote:

In D41825#970033, @zturner wrote:

So each interval has exactly 4096 blocks. Block 0 is only special in the first interval. It's still there in every other interval, it just contains regular data.

But according to the code, that can't be true. Specifically, I'm looking at llvm::msf::getFpmStreamLayout. If you look at the loop assuming that FpmBlock is 1, then it pushes index 1 as the first FPM block, adds the result of msf::getFpmIntervalLength (which always returns 4096), and pushes that. This means that the second FPM in the sequence is at index 4097, which would mean that there are 4097 blocks in front of it. Unless block at index 4096 is unused, the first interval is in fact 1 block longer.

Doesn't all of that match up exactly with the layout I posted? Paste a copy of that layout diagram in my previous comment onto the end of itself. 4096 becomes "Data", 4097 becomes "FPM1", 4098 becomes "FPM2". "Interval" is the distance between two consecutive blocks of the same FPM. It's always 4096. So FPM1 is on blocks 1, 4096 + 1, 4096*2 + 1, 4096*3 + 1, etc. The blocks immediately before that (0, 4096, 4096*2, 4096*3, etc) are just data, with the only exception being block 0, which is the super block.

In D41825#970033, @zturner wrote:

So each interval has exactly 4096 blocks. Block 0 is only special in the first interval. It's still there in every other interval, it just contains regular data.

But according to the code, that can't be true. Specifically, I'm looking at llvm::msf::getFpmStreamLayout. If you look at the loop assuming that FpmBlock is 1, then it pushes index 1 as the first FPM block, adds the result of msf::getFpmIntervalLength (which always returns 4096), and pushes that. This means that the second FPM in the sequence is at index 4097, which would mean that there are 4097 blocks in front of it. Unless block at index 4096 is unused, the first interval is in fact 1 block longer.

Oh hey also, should this go in 6.0?

I'm assuming you'll want approval from someone else too, but this looks good to me, and is proven to fix my bug.

Makes sense! Thanks for adding a test btw.

Abandoned in favor of D41742.

Yeah, that's the callstack I'm seeing. And I'm all for as many asserts as we can add to guarantee safety.

Just kidding it already does that?

OOOOH That makes so much sense. I was wondering how you could have 2 FPMs at the start but only one every stride after that, but never connected that to this...

In the process of writing this comment, I rewrote it like 8 times whenever I discovered something. So sorry if this reads disjointly :)

This actually took me 3-4 days to track down, but I'll try to explain as clearly as I can :)

Thanks Reid! Would you mind submitting this for me?

llvm::IntegerType::get(getLLVMContext(), 128) -> Builder.getInt128Ty()

Moved implementation to X86_64 specific code, as according to Microsoft docs this function isn't supported on X86 or ARM.