LGTM with one comment. This is great, thanks!

LGTM, Cheers!

LGTM

Thanks! I think your tests don't actually test the added code, can you check if you can parse a mapping like ios_tvos in the constructed SDK settings?

I think we would still want to diagnose any mismatches between class properties that override a class property in the superclass. do you think that would be useful? You can adjust the lookup to take that into account.

This change caused the following regression on the EXPENSIVE_CHECKS=on green dragon CI, where clang is unable to compile compiler-rt for armv7:

@vitalybuka Hi, this change caused some test failures on green dragon apple CI. This is the bot: https://green.lab.llvm.org/green/job/clang-stage1-RA/. The following tests are failing:

@vitalybuka This change caused a regression on Darwin CI for tsan -,
This is the CI job: https://green.lab.llvm.org/green/job/clang-stage1-RA, and the build: https://green.lab.llvm.org/green/job/clang-stage1-RA/26354/
ThreadSanitizer-x86_64.Darwin.dlopen.cpp is now failing because the initializer that you added runs before flags is initialized when it's being dlopened . Could you take a look at this issue?

This looks good to me! I agree, an enum class would be cleaner though.

In D114968#3168123, @dexonsmith wrote:

In D114968#3167519, @jansvoboda11 wrote:

Assuming the filesystem doesn't change during dependency scanning, this change keeps the consistency between stat/read calls for minimized files.

Thinking about it some more though, the original reason for reading files eagerly (even for stat calls) was most likely to avoid inconsistencies when the file changes on the filesystem between stat and read. Is that correct, @arphaman? If so, I think I should abandon this patch.

Agreed; that consistency is important. I don't think stat should be decoupled from reading the file.

Sorry for delay, this LGTM. Thanks for doing this cleanup!

I will commit this on behalf of @beccadax as she doesn't have commit access yet.

Looks good to me. Can you post the patch with the full context next time so that it's easier to review?

LGTM, thanks

In D108976#2979867, @arphaman wrote:

Could you add a test case for this change? Maybe a unit test?

Could you add a test case for this change? Maybe a unit test?

Thanks, LGTM

I think this approach makes sense for now. It's unfortunate that the constructor of FullSourceLoc is unable to validate this requirement, do you know how many clients that you describe as modifying their ID to make them valid are there?

I think this should be fixed in 40d2d0c41298f1d8a178216e2534b29e3128cf37.

I think this should be fixed if I add -fuse-ld= to the invocation. Let me commit that fix to see if it's fixed.

In D105960#2897245, @leonardchan wrote:

Hi. I think this patch may have led to the test failure we're seeing on our clang builders (https://luci-milo.appspot.com/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8840986998954824016):

Command Output (stderr):
--
/b/s/w/ir/x/w/llvm-project/clang/test/Driver/darwin-ld.c:146:29: error: LINK_VERSION_MIN_MACABI: expected string not found in input
// LINK_VERSION_MIN_MACABI: "-maccatalyst_version_min" "13.1.0"
                            ^
<stdin>:5:15: note: scanning from here
 "/usr/bin/ld" "-demangle" "-lto_library" "/b/s/w/ir/x/w/staging/llvm_build/lib/libLTO.dylib" "-dynamic" "-arch" "x86_64" "-macosx_version_min" "10.6.0" "-o" "a.out" "-lcrt1.10.6.o" "/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld.c.tmp.o" "-lSystem"
              ^
<stdin>:5:118: note: possible intended match here
 "/usr/bin/ld" "-demangle" "-lto_library" "/b/s/w/ir/x/w/staging/llvm_build/lib/libLTO.dylib" "-dynamic" "-arch" "x86_64" "-macosx_version_min" "10.6.0" "-o" "a.out" "-lcrt1.10.6.o" "/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld.c.tmp.o" "-lSystem"
                                                                                                                     ^

Input file: <stdin>
Check file: /b/s/w/ir/x/w/llvm-project/clang/test/Driver/darwin-ld.c

-dump-input=help explains the following input dump.

Input was:
<<<<<<
             1: Fuchsia clang version 13.0.0 (https://llvm.googlesource.com/a/llvm-project ae4e8f7d528f650e0f2730a7f166fb0ef7e31588) 
             2: Target: x86_64-apple-darwin10 
             3: Thread model: posix 
             4: InstalledDir: /b/s/w/ir/x/w/staging/llvm_build/bin 
             5:  "/usr/bin/ld" "-demangle" "-lto_library" "/b/s/w/ir/x/w/staging/llvm_build/lib/libLTO.dylib" "-dynamic" "-arch" "x86_64" "-macosx_version_min" "10.6.0" "-o" "a.out" "-lcrt1.10.6.o" "/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld.c.tmp.o" "-lSystem" 
check:146'0                   X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:146'1                                                                                                                          ?                                                                                                                                                                possible intended match
             6: Fuchsia clang version 13.0.0 (https://llvm.googlesource.com/a/llvm-project ae4e8f7d528f650e0f2730a7f166fb0ef7e31588) 
check:146'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             7: Target: x86_64-apple-macosx10.7 
check:146'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             8: Thread model: posix 
check:146'0     ~~~~~~~~~~~~~~~~~~~~
             9: InstalledDir: /b/s/w/ir/x/w/staging/llvm_build/bin 
check:146'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            10:  "/usr/bin/ld" "-demangle" "-lto_library" "/b/s/w/ir/x/w/staging/llvm_build/lib/libLTO.dylib" "-dynamic" "-arch" "x86_64" "-macosx_version_min" "10.7.0" "-o" "a.out" "-lcrt1.10.6.o" "/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld.c.tmp.o" "-lSystem" 
check:146'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             .
             .
             .
>>>>>>

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  Clang :: Driver/darwin-ld.c

Would you mind taking a look and sending out a fix or revert?

In D106316#2888628, @steven_wu wrote:

Looks good in general. Just one corner case that we need to decide with direction we go, following command builds arm64-ios and x86_64-ios-simulator:
clang -arch arm64 -arch x86_64 -c -o test.o test.c -mios-version-min=14
Should we document and deprecate that behavior?

Addressed review comments.

This patch depends on a small NFC commit that moves DarwinSDKInfo over to lib/Basic from lib/Driver, which isn't up on phabricator.

In D105257#2853438, @dexonsmith wrote:

Could the DarwinSDKInfo changes be tested directly with C++ unit tests? Since this stuff is in Basic it'd be nice to test it separately from the driver. If so, maybe they could also mostly land in a separate prep commit (except the update to clang::parseDarwinSDKInfo which is what changes the Driver behaviour).

Split patch and address review comments

fix typo

Sorry, took a bit longer than anticipated. I updated the patch to address reviewer's comments

Looks like this patch causes a number of issues for us, I will work with @jansvoboda11 to see if there's some way to resolve them.

Thanks for the feedback, I'll update this patch tomorrow.

I've started testing this change. I'll let you know how it looks in a few days.

I agree with Duncan, it would be good to avoid modifying the existing test cases if possible

Thanks, LGTM!

Thanks, LGTM

Thanks, LGTM

Hey, thanks for following up on this PR. I've done some more digging and I think we can remove this Darwin-specific workaround in the near future. I'm hoping to provide an update in the next few weeks.

I think this is good to go, I haven't observed any issues with this patch so far in my testing. LGTM

It might be good for @aganea to take a look as well.

Do not apply this to 'deprecated' availability annotations - the user has no way to check for deprecated as respondsToSelector will still return true even if the class has marked the method as deprecated.