Page MenuHomePhabricator

[Index] Make sure c-index-test finds libc++ on Mac
Changes PlannedPublic

Authored by ilya-biryukov on Jan 31 2019, 11:39 AM.

Details

Summary

Specifically, the version of libc++ that lives alongside the c-index-test
binary. After r348365 driver relies on a correct value of argv[0] to find
libc++ and 'c-index-test' did not pass proper argv[0] when calling the
driver.

Event Timeline

ilya-biryukov created this revision.Jan 31 2019, 11:39 AM
Herald added a project: Restricted Project. · View Herald Transcript

I'm not sure who the owners of c-index-test are, so adding people who touched the file recently or might be interested in this landing.
Suggestions for other reviewers are very welcome.

Still struggling to make one test pass: Index/index-file.cu fails with:

.../index-file.cu:8:20: error: CHECK-HOST-NOT: excluded string found in input
// CHECK-HOST-NOT: macro definition=__CUDA_ARCH__
                   ^
<stdin>:3631:48: note: found here
// CHECK: __clang_cuda_runtime_wrapper.h:77:9: macro definition=__CUDA_ARCH__ Extent=[77:9 - 77:26]

The catch is that I see this line both after and before my change when I run the corresponding invocation of c-index-test with --cuda-host-only by hand.
Any ideas why this might be happening? How do I emulate the environment of llvm-lit completely to get a repro?

Looking at D54630, it seems arphaman and dexonsmith are probalby good reviewers for this.

arphaman requested changes to this revision.Jan 31 2019, 1:42 PM

The real problem is that clang_parseTranslationUnit2 is broken, so this just fixes one client, c-index-test. All other macOS clients that call that function will still be unable to find the libc++ headers.

clang_parseTranslationUnit2 should try to infer the binary directory based on the libclang path. Libclang already does that for -resource-dir, see (CIndexer::getClangResourceDir). I think that you should set the clang path to CINdexer::getClangToolchainPath()/bin/clang, and that should fix the issue.

This revision now requires changes to proceed.Jan 31 2019, 1:42 PM
  • Revert "changes to main"
  • Compute the path in parseTranslationUnit2

@arphaman, thanks for pointing this out. It's definitely nice to change stuff only in one place.
There are two issues still pending:

  • index-file.cu still fails, I haven't looked into it closely yet, any idea why that might be happening?; repro is simple, just patch this in locally and run the test.
  • I'm not sure it's safe to use temporary storage for argv[0] in clang_parseTranslationUnit2. The resulting translation units probably holds references to argv and is free to read the data later. A simple and safe alternative would be to store this string in CIndexer and return a reference here. Does that LG?

@arphaman, thanks for pointing this out. It's definitely nice to change stuff only in one place.
There are two issues still pending:

  • index-file.cu still fails, I haven't looked into it closely yet, any idea why that might be happening?; repro is simple, just patch this in locally and run the test.

No idea why it fails, I'll try to take a look.

  • I'm not sure it's safe to use temporary storage for argv[0] in clang_parseTranslationUnit2. The resulting translation units probably holds references to argv and is free to read the data later. A simple and safe alternative would be to store this string in CIndexer and return a reference here. Does that LG?

Yes, that would be better. Let's store the string in CIndexer like we do for toolchain path and the resource dir.

  • Move the string to CIndexer.
  • Simplify code
arphaman accepted this revision.Feb 8 2019, 7:02 PM

LGTM.

This revision is now accepted and ready to land.Feb 8 2019, 7:02 PM
ilya-biryukov planned changes to this revision.Feb 11 2019, 1:29 AM

Still need to figure out why the test fails and fix it before submitting the change.

ilya-biryukov, ping? :-)

Lost this one after vacation, sorry.
Still need to figure out why the test is failing. Any help appreaciated