Skip to content

Conversation

@qiongsiwu
Copy link

llvm#164345 merge conflict resolution with next introduced two issues.

  1. It did not correctly pick up the logic to set StableDir for CompilerInstanceWithContext.
  2. It did not correctly setup the action controller for CAS.

This PR fixes both issues.

@qiongsiwu
Copy link
Author

We have tests in swift that catch these two bugs, but I don't think we have any in next. I can look into adding some test cases if that is desired.

@qiongsiwu
Copy link
Author

@swift-ci please test llvm

@qiongsiwu
Copy link
Author

The two failing tests were failing already before this PR.

Failed Tests (2):
    Clang :: Sema/attr-counted-by-void-ptr-gnu.c
    LLVM :: tools/dsymutil/ARM/cas-gmodule.test

Copy link

@jansvoboda11 jansvoboda11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please add a test for these. I understand that the missing call to createActionController() didn't cause a test failure - you probably need a new test for this with CAS and by-name scan. But the StableDirs being a local instead of member now surprises me, that should definitely trigger some existing (upstream) test in LLVM, no?

@qiongsiwu
Copy link
Author

@swift-ci please test llvm

@qiongsiwu
Copy link
Author

qiongsiwu commented Nov 17, 2025

I am adding a stable dir test upstream with llvm#168143.

But the StableDirs being a local instead of member now surprises me, that should definitely trigger some existing (upstream) test in LLVM, no?

No because no testing upstream was there tests by-name lookup.

@jansvoboda11
Copy link

I am adding a stable dir test upstream with llvm#168143.

But the StableDirs being a local instead of member now surprises me, that should definitely trigger some existing (upstream) test in LLVM, no?

No because no testing upstream was there tests by-name lookup.

I see. Is the upstream test actually broken without this PR? Or what other consequences are there without this PR?

@qiongsiwu
Copy link
Author

qiongsiwu commented Nov 17, 2025

I am adding a stable dir test upstream with llvm#168143.

But the StableDirs being a local instead of member now surprises me, that should definitely trigger some existing (upstream) test in LLVM, no?

No because no testing upstream was there tests by-name lookup.

I see. Is the upstream test actually broken without this PR? Or what other consequences are there without this PR?

The upstream test passes upstream, but it would be broken if it was in next without this PR. We set StableDirs correctly upstream. See https://github.com/llvm/llvm-project/blob/3f60d220514c4be00e548a17a85c2fa8fa89cc35/clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.cpp#L762. I introduced the bug when I was merging the change into next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants