This is an archive of the discontinued LLVM Phabricator instance.

[SourceManager] Skip module maps when searching files for macro arguments
ClosedPublic

Authored by jkorous on Aug 19 2020, 10:06 AM.

Details

Summary

I have a test-case for this issue but it's pretty convoluted - we basically found this because clang_annotateTokens in libclang wasn't
attaching RefDeclExpr to macro argument identifiers. This was happening for a source file that includes module that has external submodule defined in its modulemap.

Diff Detail

Event Timeline

jkorous created this revision.Aug 19 2020, 10:06 AM
jkorous requested review of this revision.Aug 19 2020, 10:06 AM

Could you please add a comment stating why it's necessary to skip the module map files? Is the problem caused by the module map file that's included in the include hierarchy when it's an external module?

Is there any way to reduce the convoluted test-case and add it to the patch?

I could be missing something but it seemed that skipping module maps when we're searching for source locations is obviously the right thing to do. Don't really know if repeating what the code does in the comment would bring any value.

The reproducer I have is at libclang API level and involves module and external submodule which seems pretty far (both conceptually and code-path-wise) from macro args cache initialization.

The reproducer dir looks like this:

> find .
.
./modulefoo
./modulefoo/module.map
./modulefoo/dyld.h
./modulefoo/dyld.modulemap
./modulefoo/loaderfoo.h
./code.c
> cat code.c
#include "modulefoo/loaderfoo.h"
#define FOO234878786(a) (a + a)
const int a = 1;
const int b = FOO234878786(a);
> cat modulefoo/module.map
module MachOFoo [system] [extern_c] {

  extern module dyld "dyld.modulemap"

  module loaderfoo {
    header "loaderfoo.h"
    export *
  }

}

And this is the way to see observable difference:

c-index-test -test-annotate-tokens=code.c:4:1:4:100 code.c -fmodules -fmodules-cache-path=/tmp/mcp -x c
This comment was removed by jkorous.

Okay, that's fair. Is there any way to trigger the computeMacroArgsCache outside of libclang, to create a clang-specific test case?

jkorous updated this revision to Diff 292928.Sep 18 2020, 6:05 PM

Attempt to write a unit test - so far can't reproduce even with unpatched master :(

The only place where SourceManager::computeMacroArgsCache is used is SourceManager::getMacroArgExpandedLocation.

I literally just copied and slightly modified existing unit-test which doesn't work - any help would be appreciated.

Friendly ping.
I'd like to land this sooner than later - I agree it'd be great to have a regression test but my (possibly biased) opinion is that the change is also simple and obvious enough that we could land it without a test.

arphaman accepted this revision.Sep 28 2020, 11:02 AM

Hmm if this unit tests doesn't reproduce it then you don't need to add it. But no further objections from me on this change.

This revision is now accepted and ready to land.Sep 28 2020, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2020, 12:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dexonsmith added inline comments.Oct 22 2020, 1:04 PM
clang/lib/Basic/SourceManager.cpp
1794–1799

It seems unintentinoal to copy out the FileInfo here. Should this be a reference?

auto &File = Entry.getFile();