LGTM.

Plus replacing a few std::map<string,...> with llvm::StringMap

LGTM, after you fix the nits I mention inline.

This LGTM, assuming @echristo is happy.

In D81905#2105843, @kbobrovs wrote:

Yes, I infer semantics properties. I use demangler to implement lowering of "C++ intrinsics" - Itanium-mangled C++ name (e.g. templated function instantiation) - for my SPIRV-based target. Lowering depends on the name, template parameter values and argument types (if overloaded). https://github.com/intel/llvm/pull/1881

If its to infer some semantic property of the symbol then there probably are better ways of expressing that, like getting the frontend to add metadata or a function attribute or something.

It is not obvious to me that metadata + attributes is better than demangling (as long as it implemented correctly) - with demangling implementation is more self-encapsulated and simpler. But that's pretty much orthogonal to this code change.

Anything that was "planned to be removed in 4.0" is now supported "forever" until we make an explicit choice to break backward compatibility.

LGTM.

LGTM, with one more comment inline.

LGTM.

LGTM once @aprantl is happy.

I have a couple of drive-by suggestions, up to @dang and @Bigcheese whether to incorporate them.

LGTM, but next we should get the crash reproductions working with in-process -cc1.

In D80383#2068596, @dang wrote:

I made the decision to make all relative offsets relative to the start of the AST block rather than their own sub-block or a neighboring block, in order to prevent adding complexity in ASTReader. My reasoning that it would be best if the reader didn't have to keep track of a bunch of offsets inside the bitcode at all times, but that it could just remember the offset of the AST block once and always use that for relative offsets. I agree that conceptually making the sub-blocks relocatable too would have been nice, but I figured it wasn't worth the extra complexity in the reader.

Thanks for working on this! I have a number of suggestions inline.

In D80808#2066786, @dang wrote:

Can you commit this for me?

LGTM.

In D79916#2039842, @dexonsmith wrote:

IOW, this LGTM if Alex and Gerolf are happy.

In D79916#2039789, @echristo wrote:

I'm totally down, but you knew that already :)

Duncan: Do you have any concerns? I doubt it, but just checking.

IOW, this LGTM if Alex and Gerolf are happy.

Committed in 558db27c4971cedde1a5915f390a7897e3038e10.

LGTM. I can commit for you.

In D74813#2036966, @alexbdv wrote:

@dexonsmith - Are you OK with that ?

In D79343#2025211, @ldionne wrote:

Regarding the formatting changes, I personally think we should clang-format all of libc++, libc++abi and libunwind in one go. Then we'd be done with these small issues that add up. And we can even add the revision to .git-blame-ignore-revs.

In D79343#2023658, @CaseyCarter wrote:

I'm not going to reformat *only* my additions per the clang-format instructions - that would be silly - and I suspect that folks would clang-format all of the tests if they actually wanted them formatted, so I won't format the entire file, either.

In D77621#2002430, @dblaikie wrote:

(seems a bit questionable to rely on uintptr_t being necessarily the same type as either uint32_t or uint64_t - but maybe that's guaranteed/written down somewhere)?

@cantonios, thanks for working on this. I'll let @ldionne or others review in detail, but I have two high-level notes:

In D74813#2011083, @alexbdv wrote:

@dexonsmith what regression are you referring to ?
What this change is effectively doing now is changing the numbering of the blocks from incremental to hash-based.
So the demangler functionality remains the same (i think) - I saw that it is ignoring the (currently incrememntal) number, so changing it shouldn't make a difference.
Did I miss anything ?

In D74813#2010859, @erik.pilkington wrote:

In D74813#2010767, @alexbdv wrote:

@erik.pilkington would the hash-based numbering be OK for now ?

Feel free to drop the demangler changes for now, but I would prefer including the output of the mangled type in the symbol rather than it's hash.

@nikic, great news! Thanks for doing the detailed analysis.

In D77621#2000237, @nikic wrote:

Okay, I'm basically fine with that, if it is our stance that SmallVector should always be preferred over std::vector. Not really related to this revision, but it would probably help to do some renaming/aliasing to facilitate that view. Right now, the number of SmallVector<T, 0> uses in LLVM is really small compared to the std::vector<T> uses (100 vs 6000 based on a not very accurate grep). I think part of that is in the name, and calling it using Vector<T> = SmallVector<T, 0> and using VectorImpl<T> = SmallVectorImpl<T> would make it a lot more obvious that this is our preferred general purpose vector type, even if the stored data is not small.

In D77621#1995673, @browneee wrote:

Thanks for the revert explanation and notes, nikic.

@dexonsmith what is your current thinking on going back to the original std::vector approach?

In D77601#1974745, @browneee wrote:

Incorporated into https://reviews.llvm.org/D77621 (because this is not submitted yet, and that change now moves this code).

no-exceptions sounds a bit better to me as a feature name rather than the Clang command-line argument, although what you have is probably okay too. (Unifying sounds great! I'll let someone else sign off though.)

In D77621#1979769, @browneee wrote:

GIT_COMMITTER_NAME=Andrew Browne
GIT_COMMITTER_EMAIL=browneee@google.com

This would be my second commit. I will request access next time - thanks @dexonsmith!

In D74651#1987364, @rnk wrote:

In D74651#1987254, @aeubanks wrote:

Does the LangRef patch typically come before, after, or in the same commit? After doing some experimenting?

I guess in this case it would be best to put together a LangRef patch to land before this one, in case anyone wants to look at it alone, without the boilerplate.

Yes, I like the look of that. Can you also update the demangler to reverse this nicely? @erik.pilkington can help there. There are two copies. Usually you modify the copy in libcxxabi/ and then run a script to copy the changes into llvm/.

In D74813#1982089, @alexbdv wrote:

@erik.pilkington / @vsk / @dexonsmith - how block name = hash(parent_function_name + block_type) ?
So any blocks that are inside the same parent function + have the same type will get an incremental number to de-duplicate names.

In D77830#1979631, @alexbdv wrote:

I'll look into adding what needs to be added to have this also work for non-LTO. I was imagining It would be lot more integration.
Any other thoughts on this ?

In D77621#1979183, @browneee wrote:

@dexonsmith I am not a committer, if the last changes looks good please submit for me. Thanks!

LGTM. Seems like a reasonable new home.

LGTM.

In D70284#1752893, @bendjones wrote:

In D70284#1752811, @dexonsmith wrote:

For some reason this revision is locked down and I'm not allowed to "edit" it, which includes adding inline review comments. Can you add me as a reviewer?

Thought I did.

Thanks for your patience, I missed the updates on Friday.

In D77772#1973250, @Bigcheese wrote:

Not really. It's a static function in MemoryBuffer.cpp, and the MemoryBuffer class doesn't have a Kind member so we can't check for MemoryBufferMMapFile.

Code change LGTM.

In D70366#1970758, @jdoerfert wrote:

In D70366#1970526, @dexonsmith wrote:

In D70366#1970299, @LevitatingLion wrote:

Maybe we can add an additional string attribute when adding the noinline attribute to functions which are not marked noinline in the source code, something like "noinline-added-by-clang". I don't know if that's a legitimate use case for a string attribute, but it wouldn't be very invasive. What do you think?

Another option (not sure if it's better) would be to add a noopt LLVM attribute that Clang adds for -O0 instead of noinline. Two possibilities would be to update the inliner to pay attention to that as well (with special logic for flatten), or to change the always-inliner to add noinline to anything marked noopt.

noopt == optnone? Both optnone and noinline are set in O0, so we could just not place noinline (I think).

In D77697#1969998, @compnerd wrote:

@dexonsmith - yeah, sadly I dont think that there is a good way to audit that - any change to the public headers can cause issues. Furthermore, the libc headers themselves also influence this.

In D70366#1970526, @dexonsmith wrote:

In D70366#1970299, @LevitatingLion wrote:

Maybe we can add an additional string attribute when adding the noinline attribute to functions which are not marked noinline in the source code, something like "noinline-added-by-clang". I don't know if that's a legitimate use case for a string attribute, but it wouldn't be very invasive. What do you think?

Another option (not sure if it's better) would be to add a noopt LLVM attribute that Clang adds for -O0 instead of noinline. Two possibilities would be to update the inliner to pay attention to that as well (with special logic for flatten), or to change the always-inliner to add noinline to anything marked noopt.

In D70366#1970299, @LevitatingLion wrote:

Maybe we can add an additional string attribute when adding the noinline attribute to functions which are not marked noinline in the source code, something like "noinline-added-by-clang". I don't know if that's a legitimate use case for a string attribute, but it wouldn't be very invasive. What do you think?

In D70366#1970375, @jdoerfert wrote:

TBH, I would issue a warning if we see flatten in O0 that says this will not work and be done with it.

In D77621#1968647, @browneee wrote:

Do we want to increase the complexity of SmallVector somewhat? or do we want to keep the limit and affirm SmallVector is for small things?

This SGTM in principle, but the patch makes me wonder: which other headers from the non-modularized C runtime change ownership?

Requesting changes just to be sure we consider the other options. I don't think it's good that SmallVector is no longer useful for large byte streams; I would prefer to fix that then stop using the type.

This is thanks to a commit of mine that shaved a word off of SmallVector. Some options to consider:

In D77586#1967052, @erik.pilkington wrote:

This also adds the same feature to ObjC blocks under the assumption that ObjC wishes to follow the C standard in this regard.

SGTM - thats generally been the idea for blocks.

In D77321#1958052, @arphaman wrote:

LGTM.

In D76594#1957452, @DmitryPolukhin wrote:

@rsmith, @dexonsmith, @jdoerfert could you please take a look to this diff?
If you think that there are other reviewers who might have more context AST persistence, please add them.

In D77058#1950019, @tejohnson wrote:

I think this is a good approach, rather than a per-function attribute, since as mentioned this will be preserved through inlining.
@dexonsmith, does that seem reasonable to you? I missed the original patch and agree with you that we don't want to fix this in LTO by passing the option through to LTO.

@fhahn, please revert, this isn't how we usually pass options in LTO.

In D76290#1930134, @yln wrote:

I don't understand yet what this enables. Please extend your commit message to elaborate a bit more for people (like me) who aren't familiar with libcxx testing.
Does this enable deriving new tests from existing source files or running existing tests (.{pass,fail,sh}.cpp) in a different way without needing to modify those dozens/hundreds of files?

The change itself looks innocent enough and I don't want to block you on adding a test if there are currently none at all. LGTM.

It would also be good to confirm that this isn't expensive in the normal case (one-level substitutions). Have you timed running the libc++ tests with and without this patch?

This looks really cool. Can you add tests for this feature, somewhere inside llvm/utils/lit/tests?

This SGTM. Ideally we'd confirm the compile-time explosion is fixed before landing this though.

In D72068#1906074, @ekatz wrote:

In D72068#1906010, @dexonsmith wrote:

Given the contention around direction, I would suggest that you seek consensus via an RFC on llvm-dev before pinging the patch any further.

It is the first thing that I did, but I was asked to post the patch for review. I agree that there should have been more discussion in the mail, but it seems stranger (and with not much response) to ping a mail thread.
Please refer to http://lists.llvm.org/pipermail/llvm-dev/2020-January/138710.html for further details.

In D72068#1905933, @ekatz wrote:

In D72068#1889825, @rnk wrote:

I'm still basically not in favor of this. If another contributor thinks it's a great idea, I wouldn't go so far as to block this change, but I don't plan to do any further review.

Fair enough, I respect your opinion.
However, I still think it is a very welcoming change (for the many reasons listed in the "summary"), and I hope other reviewers will see the benefits as I see them, and approve this change.

In D75395#1901859, @Bigcheese wrote:

In D75395#1901778, @dexonsmith wrote:

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

I had another thought. What if for the explicitly built "system" modules:

• If -Wsystem-headers is on, leave them as user modules in the explicit build.
• If -Wsystem-headers is off, turn off all diagnostics in the explicit build.

Does that give the right semantics, or is there something subtly different?

I considered this, but decided against it because I wanted the implicit and explicit builds to be as similar as possible, and reduce the amount of changes made to the original command line. There's a lot of code in Clang dealing with system files, and I'm not 100% sure that -Wno-everything would be equivalent.

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

Okay, LGTM then, as long as...

In D75213#1898320, @aprantl wrote:

I'm inclined to try to commit this and see if we get any "describes" assertion reports.