LGTM after you add the testcase I mention.

LGTM, but I have a suggestion inline for another approach.

Is warning output suppressed? If so, should we just/also disable all warnings? (IIRC, the flag is -w.)

In D63048#1558806, @sylvestre.ledru wrote:

@dexonsmith ping?

In rL336869#663240, @twoh wrote:

Hello, can you please explain a bit more about this change? What kind of redundant outputs avoided by this? Thanks!

In D62742#1540426, @dblaikie wrote:

I don't fully have the direction you've in mind/that this is a part of in my head - but roughly enough to give a thumbs-up here :)

In D62428#1525189, @mclow.lists wrote:

In D62428#1519664, @jfb wrote:

In D62428#1518488, @mclow.lists wrote:

The deletions are fine, the doc updates are good, but I think deprecation warnings are not.
Things that are going to happen in the future (a year from now, say) or never (if people don't update their tools) don't belong in the warning spew for every single build

In D60713#1523315, @aprantl wrote:

Prior to this patch Function declarations may not have a !dbg attachment, because there was no use-case for it. With call site debug info, I think it would make sense to revisit this rule. When I made my previous comment, I thought that the updated rule could be:

(1) function definitions may have a distinct DISubprogram attached (as before)
(2) function declarations may have a unique DISubprogram attached (new)

If I read the code in Verifier.cpp correctly, then it is legal for a unique DISubprogram to not have a unit, which would cause the declaration-DISubprograms to be uniqued during LTO.

so the (2) should probably be:

(2) function declarations may have a unique DISubprogram attached and it may not have a unit: field

I've CC'ed Duncan in case he can catch a logic error with this reasoning.

In D62618#1521964, @EricWF wrote:

Changing the triviality of a type can be ABI breaking because it will change the calling conventions the compiler uses.
We have hacks in pair to keep it non-trivial on some platforms for this reason.

This doesn’t look okay to me, because this would prevent building for Darwin when running on Windows. I added a couple of reviewers that have Windows experience and might have ideas for how to fix the tests without breaking the portability of the driver.

In D62228#1512483, @EricWF wrote:

In D62228#1511999, @mclow.lists wrote:

There was a discussion on the mailing list that (to my mind) petered out with no particular conclusion as to whether or not this was a good idea.

Louis and I talked more offline and there was some consensus that this minimal change was more acceptable.
But if there are additional concerns they can be raised here.

LGTM too.

In D61547#1497555, @MaskRay wrote:

In D61547#1497030, @dexonsmith wrote:

In D61547#1495855, @MaskRay wrote:

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

I used llvm-as without this patch to generate it. I added anither test to check the upgrader correctly adds i8* null

What's the problem with using the pre-existing file?

test/Bitcode/upgrade-global-ctors.ll.bc uses zeroinitializer, it doesn't check if AuoUpgrader adds i8* null as the third field. So I improved the test a bit and made it clear both globals_ctors and global_dtors are upgraded.

In D61547#1495855, @MaskRay wrote:

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

I used llvm-as without this patch to generate it. I added anither test to check the upgrader correctly adds i8* null

In D61547#1495339, @rnk wrote:

Presumably test/Bitcode/upgrade-global-ctors.ll.bc should remain unchanged, i.e. not be regenerated? We want it to use the pre-upgraded, two field form.

+bruno

Hmm; it doesn't seem safe to obsolete the 2-field form if we don't have a working bitcode upgrade from 2-field to 3-field.

In D61165#1492724, @rjmccall wrote:

In D61165#1492582, @erik.pilkington wrote:

Duncan pointed out eel.is/c++draft/class.init#class.base.init-13, which appears to claim that initialization ends with the execution of the constructor, excluding temporary destructors.

(13) In a non-delegating constructor, initialization proceeds in the following order:
/* list of steps... */
(13.4) Finally, the compound-statement of the constructor body is executed.

John: do you agree with this analysis?

That's talking about constructor bodies.

The 3-field form was introduced by D3499 in 2014 and the legacy 2-field
form was planned to be removed in LLVM 4.0

+rnk

Shouldn't this just send DYLD_LIBRARY_PATH through to the second stage, so that ar and ranlib use the just-built libLTO.dylib?

LGTM.

In D60924#1473176, @steven_wu wrote:

In D60924#1473166, @dexonsmith wrote:

In D60924#1473161, @steven_wu wrote:

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

Maybe we can split out the function to generate the filename, and add a unit test for that?

That almost looks like a test for path::append. Not sure if there is much value to add that.

In D60924#1473161, @steven_wu wrote:

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

Is there a good way to test this?

In D60748#1472829, @rjmccall wrote:

I suspect Darwin also doesn't want to take this. We care very little about 32-bit Intel, and part of caring very little is not wanting to spend any effort dealing with the ramifications of ABI breaks. That would apply both to the rule in general and to the vector rule specifically. @dexonsmith, agreed?

I wonder if we could add a module flag for the TLI, and then store that in the summary. Would it work for both implementations in that case?

+aprantl

In D60233#1467092, @trixirt wrote:

A comment by whisperity in the origin WIP did not seem to be addressed.
Have you looked at IWYU https://github.com/include-what-you-use/include-what-you-use ?
The end goals of clang-scan-deps and iwyu seem similar so their implementation problems would also be similar.
The iwyu problems doc is https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYUIsDifficult.md

In D55463#1451166, @tschuett wrote:

Are you planning to do this recursively?
The minimizer does not help much for the following example, while Sema.h contains 10,000 lines of useless code.

#include "clang/Sema/Sema.h"

int foo() {}

I'm in favour of this.

Committed r357038.

Committed r357037.

+Erik and Alex, can you take a look at this?

LGTM.

LGTM!

Rebase.

In D59377#1431203, @jkorous wrote:

In D59377#1430002, @dexonsmith wrote:

... since I noticed that FileManager ...

This kind of implies that we should move the comment from FileManager constructor implementation to the header thus making it an explicit part of the interface contract.

// If the caller doesn't provide a virtual file system, just grab the real
// file system.

Maybe https://reviews.llvm.org/D59388 would be the right place to do it.

Document the default VFS used by the FileManager.

In D59388#1431314, @jkorous wrote:

Hi Duncan, thanks for working on better interfaces in clang!

I am just wondering - is it safe to have the lifetime of a single object on heap managed by two different IntrusiveRefCntPtr instances?

I slightly simplified the code for CompilerInstance::createFileManager (replaced with an assert), since I noticed that FileManager always constructs its own VFS if it isn't passed one.

In D58514#1428567, @ahatanak wrote:

Sorry, I misread the code. If you change the parameter type of bar to BlockTy, the code crashes. If the type is id, it doesn't crash because IRGen copies the block to the heap in foo before passing it to bar.

In D58514#1428520, @ahatanak wrote:

That code doesn't crash because the block is retained at the entry of bar and ARC optimizer doesn't remove the retain/release pairs in bar.

In D58514#1428434, @ahatanak wrote:

Seems like the chromium code is valid and shouldn't crash. John/Erik what do you think? The following code also crashes with this patch applied.

typedef void (^BlockTy)();

BlockTy sb;
__weak BlockTy wb;

void foo(id a) {
  auto b = ^{ NSLog(@"foo %@", a); };
  wb = b; // block isn't copied to the heap.
  sb = b; // block is copied to the heap.
}

int main() {
  auto x = [NSObject new];
  foo(x);
  sb();
  wb();
  return 0;
}

Committed in r355950. Thanks for the review.

Updated the constructor call to PCHGenerator in GenerateModuleAction::CreateASTConsumer to use BuildingImplicitModule on its own. Checking where it's set (only in compileModuleImpl), it's exactly the condition we want here.

In D59176#1424864, @jordan_rose wrote:

This commit by itself doesn't change any behavior, right?

Committed in r355778.

Committed in r355777.

LGTM too with some nitpicks (forgot to hit "submit" earlier this afternoon).

+ab, do you still know this code?