In D81865#2293146, @MaskRay wrote:

In D81865#2293066, @dblaikie wrote:

In D81865#2293059, @MaskRay wrote:

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

Judging by a cursory glance at Phab's view of the delta ( https://reviews.llvm.org/rG4b64ce7428b66cacfe74dbd9dbc29aff6dfb84af ) it /looks/ like it wasn't too different. Mostly picking up upstream changes that added "DEFERRABLE"? (I think Phab uses light green for "this changed, but only because of upstream changes" and dark green is the actual patch changes?)

Sorry for the noise. What I saw previously was a mere difference in the DIAG macro and the new isDeferable... Maybe Phab presented the diff between two Diffs to me. The updated view seems good.

Throwing this back into the review queue.

Updated to use a struct instead of a union for the actual table in an effort to avoid UB.

Ping for an opinion on the recent discussion of how to move forward with this patch.

Ping for an opinion on the recent discussion of how to move forward with this patch.

In D81865#2176589, @dblaikie wrote:

I believe this falls under the (using cppreference ( https://en.cppreference.com/w/cpp/language/union ) , since it's a bit easier to read) UB clause: " It's undefined behavior to read from the member of the union that wasn't most recently written. Many compilers implement, as a non-standard language extension, the ability to read inactive members of a union."

Last member written to was the "StringTable" member, but then it's read from the "String" member, so that'd be UB. Commonly supported, but UB - not sure if we have a general statement that we depend on this behavior in LLVM, even though it's non-standard (but it's possible that we do make such an assumption about the compiler that's building LLVM). It'd be nice to avoid that, though - and not too difficult, I think - I /believe/ it's valid to take a pointer to an object, cast it to char*, compute a pointer to some specific member and then cast it back to the right type and access. But I could be wrong there. @rsmith would be the person to give an authoritative answer.

In D81865#2176014, @bkramer wrote:

Nice, those relocations have annoyed me for years. I'm worried about whether the way you're accessing StaticDiagInfoDescriptionStringTable might be undefined behavior. I won't block this change on that though.

Ping.

In D75243#2169173, @ldionne wrote:

In D75243#2168014, @dmajor wrote:

In our builds with the 10.12 SDK and MACOSX_DEPLOYMENT_TARGET of 10.11, this is producing

Hmmm. There's a few interesting things here. First, I'd like to understand why you're building the dylib with a deployment target of 10.11 -- we usually make the assumption that people building libc++ are vendors, and we take for granted that they're building for new-ish platforms. For example, at Apple, we only build the dylib for the latest OS because that's what the dylib is going into, so using an older deployment target just doesn't make sense. If you have another use case, it might be useful to know about it for the future.

FWIW, an in-progress tree for the proposed move for all the architecture-specific builtins is:

Cool, thank you! Could you also land this for me? I do not have commit privileges. Thanks!

Thanks for the pointer @craig.topper ! I've updated the patch to use LessRecordByID. I've also made some small changes to how the code selects the correct macro to output and done a better job converting the builtins by using an automated script, available at https://gist.github.com/froydnj/1192911af46661fb62e724957099d4e8 . I did have to touch up the comments a bit, but the more-automated conversion pointed out several errors that my attempt to use the preprocessor had run into.

Not sure who needs to review this; picking a familiar face. ;)

I'd like to apply the same sort of techniques to the builtin tables, which are enormous, but this is baby steps.

Also, I am the farthest thing from the owner of this code (I don't even have commit access!), so I don't think I'm the right person to review this.

In D70910#1765583, @dmajor wrote:

The extra setup/teardown work for Windows relative to Linux is unfortunate. How would you feel about using an SRWLock instead?

Wow, thanks for the fast review! @rnk , could you please commit this for me? I do not have commit access. Thanks!

Thanks for the review, @mstorsjo. Would you please land this for me? I don't have commit access.