colden (Colden Cullen)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 20 2017, 12:14 PM (43 w, 3 d)

Recent Activity

Feb 26 2018

colden added a comment to D43700: Emit proper CodeView even when not using the cl driver..

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

Feb 26 2018, 11:15 AM
colden added a comment to D43700: Emit proper CodeView even when not using the cl driver..

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.

Feb 26 2018, 11:04 AM

Feb 23 2018

colden added a comment to D43700: Emit proper CodeView even when not using the cl driver..

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.

Feb 23 2018, 4:39 PM
colden added a comment to D43700: Emit proper CodeView even when not using the cl driver..

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

Feb 23 2018, 4:01 PM

Jan 31 2018

colden committed rLLD323895: [PDB] Fix test failures due to expected warning not matching actual warning text.
[PDB] Fix test failures due to expected warning not matching actual warning text
Jan 31 2018, 10:19 AM
colden committed rL323895: [PDB] Fix test failures due to expected warning not matching actual warning text.
[PDB] Fix test failures due to expected warning not matching actual warning text
Jan 31 2018, 10:19 AM
colden committed rLLD323893: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.
[LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error
Jan 31 2018, 9:50 AM
colden committed rL323893: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.
[LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error
Jan 31 2018, 9:50 AM
colden closed D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.
Jan 31 2018, 9:50 AM
colden closed D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.
Jan 31 2018, 9:50 AM

Jan 25 2018

colden added a comment to D42496: [CMake] Fix Bug Report URL.

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.

Jan 25 2018, 11:19 AM

Jan 24 2018

colden added a comment to D42496: [CMake] Fix Bug Report URL.

LGTM.

Thanks for reviewing this for me!

Jan 24 2018, 11:53 AM
colden created D42496: [CMake] Fix Bug Report URL.
Jan 24 2018, 10:49 AM

Jan 23 2018

colden added a comment to D42455: Don't create hidden dllimport global values.

Confirmed, this fixes my issue! Thanks!

Jan 23 2018, 6:42 PM
colden added a comment to D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.

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

Jan 23 2018, 2:56 PM

Jan 17 2018

colden updated the diff for D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.

Rebased on top of the timer change

Jan 17 2018, 3:38 PM
colden added a comment to D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.

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...

Jan 17 2018, 3:01 PM
colden updated the diff for D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.

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

Jan 17 2018, 1:54 PM
colden updated the diff for D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.

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

Jan 17 2018, 1:18 PM
colden added a comment to D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.

Works for me! Update incoming.

Jan 17 2018, 1:17 PM
colden added a comment to D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.

Ok, that makes sense.

Jan 17 2018, 12:56 PM
colden created D42188: [LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error.
Jan 17 2018, 10:44 AM

Jan 16 2018

colden added inline comments to D42010: [LLD][COFF] Report error when file will exceed Windows maximum image size (4GB).
Jan 16 2018, 9:26 AM
colden added a comment to D42010: [LLD][COFF] Report error when file will exceed Windows maximum image size (4GB).

Bump, could someone submit this for me?

Jan 16 2018, 9:18 AM

Jan 12 2018

colden added a comment to D42010: [LLD][COFF] Report error when file will exceed Windows maximum image size (4GB).

Thanks for the quick reviews!

Jan 12 2018, 4:50 PM
colden updated the diff for D42010: [LLD][COFF] Report error when file will exceed Windows maximum image size (4GB).

Turns out you can do the easier thing

Jan 12 2018, 4:42 PM
colden updated the diff for D42010: [LLD][COFF] Report error when file will exceed Windows maximum image size (4GB).

Switch error message to use decimal format from hex.

Jan 12 2018, 4:20 PM
colden added inline comments to D42010: [LLD][COFF] Report error when file will exceed Windows maximum image size (4GB).
Jan 12 2018, 3:30 PM
colden created D42010: [LLD][COFF] Report error when file will exceed Windows maximum image size (4GB).
Jan 12 2018, 1:56 PM
colden added a comment to D41825: [Docs][PDB] Update MSF File documentation.

Thank you Zach!

Jan 12 2018, 1:44 PM

Jan 11 2018

colden added a comment to D41825: [Docs][PDB] Update MSF File documentation.

Ping, could someone please submit this for me?

Jan 11 2018, 1:33 PM

Jan 10 2018

colden added a comment to D41825: [Docs][PDB] Update MSF File documentation.

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

Jan 10 2018, 9:54 AM
colden updated the diff for D41825: [Docs][PDB] Update MSF File documentation.

nth time's the charm!

Jan 10 2018, 9:04 AM
colden added inline comments to D41825: [Docs][PDB] Update MSF File documentation.
Jan 10 2018, 8:46 AM

Jan 9 2018

colden added a comment to D41825: [Docs][PDB] Update MSF File documentation.

Updated with my newer, better understanding of the file layout

Jan 9 2018, 6:09 PM

Jan 8 2018

colden updated the diff for D41825: [Docs][PDB] Update MSF File documentation.

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 :)

Jan 8 2018, 11:45 AM
colden added a comment to D41825: [Docs][PDB] Update MSF File documentation.

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.

Jan 8 2018, 10:49 AM
colden added a comment to D41825: [Docs][PDB] Update MSF File documentation.

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.

Jan 8 2018, 10:40 AM
colden added inline comments to D41825: [Docs][PDB] Update MSF File documentation.
Jan 8 2018, 10:26 AM
colden created D41825: [Docs][PDB] Update MSF File documentation.
Jan 8 2018, 8:50 AM

Jan 5 2018

colden added a comment to D41742: [MSF] Fix FPM interval calculation.

Oh hey also, should this go in 6.0?

Jan 5 2018, 9:28 AM

Jan 4 2018

colden accepted D41742: [MSF] Fix FPM interval calculation.

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

Jan 4 2018, 4:58 PM
colden added inline comments to D41742: [MSF] Fix FPM interval calculation.
Jan 4 2018, 4:39 PM
colden added inline comments to D41742: [MSF] Fix FPM interval calculation.
Jan 4 2018, 4:00 PM
colden added inline comments to D41742: [MSF] Fix FPM interval calculation.
Jan 4 2018, 3:47 PM
colden added a comment to D41742: [MSF] Fix FPM interval calculation.

Makes sense! Thanks for adding a test btw.

Jan 4 2018, 3:33 PM
colden abandoned D41734: [DebugInfo][PDB] Fix too many FPM blocks being written in some cases.

Abandoned in favor of D41742.

Jan 4 2018, 3:29 PM
colden added a comment to D41734: [DebugInfo][PDB] Fix too many FPM blocks being written in some cases.

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

Jan 4 2018, 1:55 PM
colden added a comment to D41734: [DebugInfo][PDB] Fix too many FPM blocks being written in some cases.

Just kidding it already does that?

Jan 4 2018, 1:32 PM
colden added a comment to D41734: [DebugInfo][PDB] Fix too many FPM blocks being written in some cases.

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...

Jan 4 2018, 1:31 PM
colden added a comment to D41734: [DebugInfo][PDB] Fix too many FPM blocks being written in some cases.

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

Jan 4 2018, 1:19 PM
colden added a comment to D41734: [DebugInfo][PDB] Fix too many FPM blocks being written in some cases.

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

Jan 4 2018, 11:04 AM
colden created D41734: [DebugInfo][PDB] Fix too many FPM blocks being written in some cases.
Jan 4 2018, 10:37 AM

Dec 13 2017

colden added a comment to D41032: [CodeGen][X86] Implement _InterlockedCompareExchange128 intrinsic.

Thanks Reid! Would you mind submitting this for me?

Dec 13 2017, 2:01 PM
colden added a reviewer for D41032: [CodeGen][X86] Implement _InterlockedCompareExchange128 intrinsic: agutowski.
Dec 13 2017, 10:44 AM

Dec 12 2017

colden added inline comments to D41032: [CodeGen][X86] Implement _InterlockedCompareExchange128 intrinsic.
Dec 12 2017, 4:02 PM
colden updated the diff for D41032: [CodeGen][X86] Implement _InterlockedCompareExchange128 intrinsic.

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

Dec 12 2017, 4:01 PM
colden updated the diff for D41032: [CodeGen][X86] Implement _InterlockedCompareExchange128 intrinsic.

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

Dec 12 2017, 2:38 PM

Dec 8 2017

colden created D41032: [CodeGen][X86] Implement _InterlockedCompareExchange128 intrinsic.
Dec 8 2017, 1:55 PM