SolaWing / xcode-build-server

a build server protocol implementation for integrate xcode with sourcekit-lsp
MIT License
426 stars 19 forks source link

Symbols from pch file not found when GCC_PRECOMPILE_PREFIX_HEADER=YES #19

Closed charshep closed 1 year ago

charshep commented 1 year ago

When GCC_PRECOMPILE_PREFIX_HEADER=YES, those symbols that are derived from the import statements contained within the project's precompiled header are not found.

I'm not sure if this qualifies as an xcode-build-server issue or a clangd issue but I thought I should start with xcode-build-server for reasons that will be made clear.


Steps to reproduce

I created a minimal project that reproduces the issue as follows:

Navigate to any of the objective C files and note that symbols defined in Foundation.h are not found.


Some background info

I found this while working on a legacy project that is configured with a pch file. This project includes both Swift and Objc files so what I normally do is I use xcode-build-server/sourcekit-lsp for Swift files and clangd for ojbc files (two different lsp servers). To export XCode's objc build information, I use xcpretty to generate a compile_commands.json that clangd can use directly in lieu of BSP.

However, after working on Pull Request #18 recently I noticed you added logic to support Objc and C family languages so I reconfigured to use sourcekit-lsp for all files, not just swift files. That's when I found the issue.


Possible solution

Note that things work correctly in both my legacy project and in my minimal test project when I switch back to using clangd/xcpretty/compile_commands.json. In looking at the difference between your .compile file and xcpretty's compile_commands.json I notice the following diff:

This line in .compile

-include /Users/charshep/Library/Developer/Xcode/DerivedData-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Intermediates.noindex/PrecompiledHeaders/SharedPrecompiledHeaders/4370315142437641375/Test_Prefix.pch

is replaced in compile_commands.json with

-include /Users/charshep/Projects/Test/Test/Test_Prefix.pch

which is the relative path of my pch file. When I manually replace the line in .compile with the line from compile_commands.json it works. I have more to say about this but I'll save that for a follow-up comment.

Please let me know if you have any questions or would like any additional info. Also, I know that English is not your native language so please let me know if there's anything in this report that I can clarify.

I've attached the objc files I used for my minimal test project.

objc_files.zip

charshep commented 1 year ago

Some further explanation on xcpretty's pch processing.

xcpretty explicitly parses the path for the project's pch file (e.g., /Users/charshep/Projects/Test/Test/Test_Prefix.pch) from the clang compile command that compiles the pch file (the clang command that immediately follows the line beginning withProcessPCH) and for each subsequent compile command, it replaces the -include <DerivedData/.../SharedPrecompiledHeaders/<some number>/Test_Prefix.pch with the parsed path, e.g., /Users/charshep/Projects/Test/Test/Test_Prefix.pch.

If you're interested in looking at the implementation in xcpretty the relevant portions are:

Parsing the projects pch file path:

Performing the pch file path substitution:

I'm not entirely certain why this was done but given that the xcpretty developers implemented this logic rather than file a bug with Apple, I decided to file a bug here first. However, as I said in my previous comment, it may be that this is a bug in clangd that Apple needs to address or it may that there's some other aspect to this that I don't understand.

SolaWing commented 1 year ago

Thanks for such a informative report. xcode outputs an invalid PCH path, very strange indeed.

Referring to the implementation of xcpretty, I have replaced the path in include with the path given in ProcessPCH. You can try the latest version.

charshep commented 1 year ago

Thanks for the quick turnaround. Unfortunately running the updated version in my test project results in a .compile file that simply contains [].

charshep commented 1 year ago

Here's the XCode build log from my test project. I'll try to look into the root cause for the [] on my end when I get a chance.

xcodebuild_test.txt

charshep commented 1 year ago

The problem seems to be due to the change introduced in commit 96e4527. With the the removal of the call to strip that was on line 55, it continues reading the remainder of the file and return li on line 57 does not get called. But since the open file descriptor has read through the entire file at this point, processing stops on return from read_until_empty_line and .compile gets written with an empty array.

I'm passing in a copy of the log file directly so in_fd is getting set to an open file descriptor for the file ( theelse clause on line 342). Not sure if that makes a difference. In the xcode log file I attached previously, the problem occurs on line 133

SwiftDriver Test normal x86_64 com.apple.xcode.tools.swift.compiler (in target 'Test' from project 'Test')

which is the first occurrence of a parsable command in that file.

charshep commented 1 year ago

After reverting the strip change, my minimal test works but unfortunately my main project does not. Different targets have different pch files set on them and with your changes only the first is read. In xcpretty it appears as though the file gets read per target.

charshep commented 1 year ago

One other note. I noticed the substituted file path is contained in single quotes '' with no escaped spaces whereas all other file paths contained in .compile are not contained in quotes but have spaces escaped with \\.

SolaWing commented 1 year ago

@charshep I have fixed the strip issue, which caused by the line contains the lineending \n. By the way, I normally use the xcode postaction hook to automatically update flags, which use the xcactivityLog format, it's fine and not contains lineendings..

After reverting the strip change, my minimal test works but unfortunately my main project does not. Different targets have different pch files set on them and with your changes only the first is read. In xcpretty it appears as though the file gets read per target.

I did replace the pch per target, for example in your test log, with the following sub:

            i = " ".join(info[3:])
            print(f"use pch {pch} for {i}")
            command = pch_capture.sub(f"-include {shlex.quote(pch)}", command)

will output: use pch /Users/charshep/Projects/Test/Test/Test_Prefix.pch for normal x86_64 objective-c com.apple.compilers.llvm.clang.1_0.compiler (in target Test from project Test), PCH line info after the file contains both arch, lang, compiler, target, and I use it as a replace match hint and it works in my multiple target demo. if this not work, can you provide a issuable log file?

One other note. I noticed the substituted file path is contained in single quotes '' with no escaped spaces whereas all other file paths contained in .compile are not contained in quotes but have spaces escaped with \.

command is a shell escaped string. both '', "", \\ all are valid escape for a path. xcode seems to prefer \ , and I use shlex.quote, which use quote to ensure escape. if this cause any problem, please contact me.

charshep commented 1 year ago

@charshep I have fixed the strip issue, which caused by the line contains the lineending \n. By the way, I normally use the xcode postaction hook to automatically update flags, which use the xcactivityLog format, it's fine and not contains lineendings..

Thanks, confirmed it's no longer resulting in [] for me as well.

I'd like to use the xactivityLog but I work on a shared project and can't modify the project file to accommodate my own personal editing environment so I still use the manual log export method.

I did replace the pch per target, for example in your test log, with the following sub:

            i = " ".join(info[3:])
            print(f"use pch {pch} for {i}")
            command = pch_capture.sub(f"-include {shlex.quote(pch)}", command)

will output: use pch /Users/charshep/Projects/Test/Test/Test_Prefix.pch for normal x86_64 objective-c com.apple.compilers.llvm.clang.1_0.compiler (in target Test from project Test), PCH line info after the file contains both arch, lang, compiler, target, and I use it as a replace match hint and it works in my multiple target demo. if this not work, can you provide a issuable log file?

Yep, my mistake regarding not performing this per target.

Regarding my legacy project still not working, I took a second look at xcpretty and noticed that the line that parses the local .pch file path from the ProcessPCH command:

PROCESS_PCH_COMMAND_MATCHER = /^\s*.*\/usr\/bin\/clang\s.*\s\-c\s(.*)\s\-o\s.*/ in parser.rb

actually parses everything between -c and -o, not simply the path that follows the -c. When I extract this same match manually from my exported log file and perform the substitution manually within my .compile file it seems to work in my legacy project.

Sorry I can't provide a log file for you to work with directly but it's only my legacy project that doesn't work now, my minimal test project works. And my legacy project unfortunately isn't something that's sharable. Aside from not having permission to do so, it's just too large a project to be useful. To give you an idea of its size, the build log exported from XCode is over 50k lines.

My hope is that the change to the pch extraction will solve it but if that doesn't work I'll try to modify my Test project to replicate the issues. In any event, I really appreciate your patience in working through all this.

command is a shell escaped string. both '', "", \\ all are valid escape for a path. xcode seems to prefer \ , and I use shlex.quote, which use quote to ensure escape. if this cause any problem, please contact me.

Yep, I figured that was the case and so far I don't see that it's a problem. Just pointing it out as an FYI (for your information).

charshep commented 1 year ago

One other thing regarding xcpretty's processing ProcessPCH. I believe it's parsing the /usr/bin/clang command that follows the ProcessPCH line, i.e., the line that follows the cd line and the Precompile of line that follows the ProcessPCH line. I don't believe it's processing the ProcessPCH itself.

charshep commented 1 year ago

Another update. I hacked xclog_parser.py to do the following:

I just started playing with it but so far it seems to have resolved my remaining issue. I'll post further updates if I find any more issues.

I'm happy to share my changes but you wouldn't want to commit them. They're very much just an experimental hack/POC (proof of concept).

SolaWing commented 1 year ago

what arguments between the -c and -o?it's a so wide match and I afraid capture unwanted content。You can give a pch directive and compilec directive as sample,and replace all security information.

In mine test project, seems there are some -I args between -c and -o, may this cause different.. but these header search paths already in CompileC command..

charshep commented 1 year ago

Actually, my test project demonstrates this as well as my legacy project. Attached are the following:

I run xcpretty with this command:

cat xcodebuild.txt |xcpretty -r json-compilation-database --output compile_commands.json

The matched string from the "compile pch header" command that is substituted (the match between -c and -o) is:

/Users/charshep/Projects/Test/Test/Test_Prefix.pch -MD -MT dependencies -MF /Users/charshep/Library/Developer/Xcode/DerivedData/Test-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Intermediates.noindex/PrecompiledHeaders/SharedPrecompiledHeaders/4370315142437641375/Test_Prefix.pch.d -iquote /Users/charshep/Library/Developer/Xcode/DerivedData/Test-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Intermediates.noindex/Test.build/Debug-iphonesimulator/Test.build/Test-generated-files.hmap -I/Users/charshep/Library/Developer/Xcode/DerivedData/Test-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Intermediates.noindex/Test.build/Debug-iphonesimulator/Test.build/Test-own-target-headers.hmap -I/Users/charshep/Library/Developer/Xcode/DerivedData/Test-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Intermediates.noindex/Test.build/Debug-iphonesimulator/Test.build/Test-all-target-headers.hmap -iquote /Users/charshep/Library/Developer/Xcode/DerivedData/Test-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Intermediates.noindex/Test.build/Debug-iphonesimulator/Test.build/Test-project-headers.hmap -I/Users/charshep/Library/Developer/Xcode/DerivedData/Test-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Products/Debug-iphonesimulator/include -I/Users/charshep/Library/Developer/Xcode/DerivedData/Test-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Intermediates.noindex/Test.build/Debug-iphonesimulator/Test.build/DerivedSources-normal/x86_64 -I/Users/charshep/Library/Developer/Xcode/DerivedData/Test-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Intermediates.noindex/Test.build/Debug-iphonesimulator/Test.build/DerivedSources/x86_64 -I/Users/charshep/Library/Developer/Xcode/DerivedData/Test-cxwpiawnfpmhvieflvvjcmiquuiw/Build/Intermediates.noindex/Test.build/Debug-iphonesimulator/Test.build/DerivedSources

It's on line 200 of xcodebuild.txt and it's what's substituted in the compile commands for DummyClass.o in both .compile and compile_commands.json.

I know it's a lot of arguments being matched and I do believe it contains some redundancies. Unfortunately I don't know much about lsp internals so I don't know why it doesn't work without this substitution being made. All I can say is that that's how xcpretty does it for whatever that's worth.

xcodebuild.txt

compile_commands.json.txt

compile.txt

charshep commented 1 year ago

@SolaWing Just noticed this got closed. Was that intentional?

SolaWing commented 1 year ago

@charshep the xcpretty add following additional flags:

-MD -MT dependencies -MF <path>

this flags is dep output flags, shouldn't affect compile and check.

-iquote <path> -I<path>

this is the header search path and will affect symbol find. but all paths already included in the CompileC command, so it shouldn't cause any problem.

Does your legacy project have any header search path, include in the pch command, but not in the CompileC command?

charshep commented 1 year ago

A couple of things. First, I don't know why I thought posting info from my test project was going to be helpful since we already established that xcode-build-server was working for that project without my local changes to xclog_parser.py. Sorry about that, I wasn't thinking clearly at the time I guess.

Second, I stashed my local changes to xclog_parser.py and basically reset everything. But after doing so, I can no longer recreate the problem. I've rebuilt the project and regenerated buildServer.json and .compile. I've done everything I know how to do to reset my editor's state. I have no idea if sourcekit-lsp caches anything persistently but I've made sure no lingering sourcekit-lsp processes remained before restarting.

So, at this point I guess I'd say we can close this. If I see anything happen again I'll do my best to narrow down the cause before posting. I think I understand both xcode-build-server and xcpretty well enough now to do that.

In answer to your question though, in looking at the args in my legacy project, the only thing I can see that's present in the ProcessPCH commands that's not in the CompileC commands (other than the local pch file path) is -MD in place of -MMD but as you say that shouldn't be relevant, and the clang docs seem to confirm it.

Thanks again so much for your help and patience.