[lldb] handle relative working directories in swiftmodule debug info #3067
Conversation
|
Would you mind turning |
when does this happen? |
At the moment it doesn't, the compiler will always prefix the current directory on a relative working directory to make it absolute. This is the first part of a series of PRs to prefix the serialized debug info in swiftmodule files. This requires treating the working directory as a relative path, as it is generally replaced with either I'm still verifying the upstream Swift change, but you can see most of it here: rmaz/swift@2c5d792 |
Not quite sure what you mean by static in this context, but I moved the function out of the anonymous namespace and added a unit test for it. Is this what you had in mind? |
| #ifndef NDEBUG | ||
| /// Provided only for unit tests. | ||
| template <typename SmallString> | ||
| void ApplyWorkingDir(SmallString &clang_argument, llvm::StringRef cur_working_dir); |
There was a problem hiding this comment.
c++ question: should this have an explicit instantiation? or does it not matter?
There was a problem hiding this comment.
this was discussed offline, the conclusion has been: explicit instantiation not needed
There was a problem hiding this comment.
This works. I originally had in mind a static member function of SwiftASTContext, but this is equivalent.
Instead of making this a template, I think we can use the base class SmallStringImpl & here.
There was a problem hiding this comment.
Actually, this doesn't work. Now release builds won't be able to build the unit test target. Can we make it a static member of Swift ASTContext instead?
There was a problem hiding this comment.
This was based on the existing code further up the header:
Would you rather the NDEBUG guard is not present? I'll update the function to be a static member fn and remove the template.
There was a problem hiding this comment.
Yes, but the test that depends on this constructor is also guarded by a #ifndef NDEBUG (line 195).
If we make the function a static member of SwiftASTContext, we can leave the tests unguarded. I think that's the cleanest approach.
There was a problem hiding this comment.
I don't see a SmallStringImpl base class anywhere though, it appears to inherit directly from SmallVector. Fine with keeping this templated for now?
There was a problem hiding this comment.
You're right, it should have been SmallVectorImpl, not SmallStringImpl. Here's an example API that works like this:
https://www.llvm.org/doxygen/namespacellvm_1_1sys_1_1fs.html#a2363fbfbd0a348f5d81fc3d3223ecae3
If using the underlying SmallVectorImpl is too cumbersome, I would just hardcode the buffer size instead of using a template. But it looks like we are only converting clang_argument to a StringRef and append characters to it.
There was a problem hiding this comment.
I see, yes, that works out fine, I've updated it now.
There was a problem hiding this comment.
Looks good to me, but let's see give @adrian-prantl a chance to look at the tests since he asked for them.
|
|
||
| single_arg_rel_path = llvm::SmallString<128>("-fmodule-map-file=module.modulemap"); | ||
| ApplyWorkingDir(single_arg_rel_path, rel_working_dir); | ||
| EXPECT_EQ(single_arg_rel_path, llvm::SmallString<128>("-fmodule-map-file=rel/dir/module.modulemap")); |
This change makes it possible to use relative working directories. Prior to this change the `path::append` will put a / between the `clang_argument` and the `cur_working_dir`, which will not correctly resolve if `cur_working_dir` is relative. This change joins the path components separately.
|
@swift-ci test |
|
Thanks Richard. Could you open a matching PR against the |
Will do, thanks for merging. |
This change makes it possible to use relative working directories. Prior
to this change the
path::appendwill put a / between theclang_argumentand the
cur_working_dir, which will not correctly resolve ifcur_working_diris relative. This change joins the path componentsseparately then concatenates with the
clang_argument.