- User Since
- Feb 9 2016, 1:33 PM (167 w, 6 h)
Mon, Apr 1
Thu, Mar 28
There's dozens of places that take the API mutex without taking the process mutex. Take Kill for example: It needs to take the API mutex, but cannot take the run lock since it will be taken by the private state thread. Another example is HandleCommand, which takes the API mutex but has no process that it could ask to lock the API mutex for it.
@clayborg, I'm not sure how that would work. There's many places that lock the process run lock without locking the target API mutex, and vice versa.
We still have this patch applied on our recently-rebased fork with no problems...
Nov 5 2018
Oct 22 2018
I suppose we could but there's a few places outside of SBProcess that also use the run lock and API mutex; personally, I prefer it to be explicit which mutex is being taken first.
Oct 18 2018
Mar 12 2018
Here's the file we use in our parameter info tests locally (that don't pass with the trunk of clang, but with your changes should now theoretically pass):
Feb 8 2018
@yvvan: The clang frontend tests (PCHPreambleTest and friends) are disabled on Windows in the makefile (I think because the VFS tests depend on linux-like paths). So running the tests on Windows without failures is encouraging but not the whole story.
Jan 15 2018
Excellent, I'll rebase and commit. Thanks everyone for your patience!
Dec 14 2017
The patch is ready to go, just needs a final approval...
Dec 8 2017
It's been a while since I was in this code, but I seem to recall the file needed to exist on disk in order to uniquely identify it (via inode). Does this break the up-to-date check?
Dec 6 2017
Dec 4 2017
Locally we've done something similar (adding a clang_getCursorPrettyPrintedDeclaration function, though without exposing the PrintingPolicy) and overhauled DeclPrinter to produce proper pretty names. This is a hack that works well for us, but can never be upstreamed since it breaks too much existing code (and some of the printing decisions are subjective). Your way is better.
Here's the final patch that fixes clang_getSkippedRegions with regions in the preamble (as well as serializing the skipped regions in the PCH file).
Dec 1 2017
Brilliant, didn't know isInPreambleFileID existed. All my tests pass for me now with that change, thanks :-)
I'll update the patch on Monday.
Nov 20 2017
Nov 16 2017
Well, it seems like preamble PCH source location translation is fundamentally broken. The entry file has a single, positive file ID. The preamble PCH is treated as an imported module, so it has a negative file ID for the part that overlaps the preamble of the entry file. That means locations in the preamble part of the entry file can have two different file IDs depending on how they are arrived at.
Alright, with my patch the c-index-test *does* correctly serialize and restore the skipped ranges; the problem is that it searches for only ranges matching the target file. When there's a preamble, it's seen as a different file than the main file, even though they're really one and the same in this case.
- Well that's odd, because the test definitely fails for me without the patch. I'm only a few days behind the trunk.
- I'm looking at your test case now. I can reproduce it even with the patch; I'm investigating what's happening.
- I've fixed the warning, thanks! Using MSVC on my end which doesn't emit that particular warning.
Nov 15 2017
Fully rebased, with a test.
Nov 8 2017
Nov 7 2017
I'll rebase the patch and add a test. Thanks for looking at this!
Oct 19 2017
Sep 20 2017
Final diff. Test passes!
Sep 14 2017
Sep 13 2017
Alright, I've changed the patch so that the preamble takes into account the BOM presence and is invalidated when it changes. This automatically fixes all clients of PrecompiledPreamble, and ensures that all SourceLocations are always consistent when using a PCH generated from the preamble.
Sep 11 2017
Final diff, will commit soon. Thanks!
Sep 8 2017
It seems there's other users of PrecompiledPreamble that would have to be fixed, yes. If we go with my original fix of taking into account the BOM in the preamble bounds, there's no way of reusing the PCH when the BOM appears/disappears. I still maintain this is a common use case for IDE-type clients. This type of performance bug is very hard to track down.
The latest patch. I think this one should do the trick :-)
Sep 7 2017
Here's an updated patch. The code required to make it work is much simpler when the BOM is simply ignored :-)
Maybe there's a third option option to remove the BOM from the buffer before passing it to clang?
Could you elaborate on your use-case a little more? Is there no way to consistently always pass buffers either with or without BOM?
Out of two options you mention discarding preamble on BOM changes seems like an easy option that is both correct and won't make a difference in performance since BOM rarely changes.
Looking at your use-case, it sounds like you'll only have 1 extra reparse of preamble, which is probably fine.
Looking at the way remapped buffers are handled, I just remembered that they must exist on the file system (at the very least, in a directory that exists) or the remapping is not taken into account. So that pretty much rules out the other approach, I think.
Sep 6 2017
Here's an updated patch that allows only the PCH to be accessed from the real FS when a VSF is present. Tests still pass.
I'll change the overlay to only allow access to the PCH.
Thanks for the response!
Sep 5 2017
I suppose we could do the overlay manually in all the tests that need it; I guess it depends on what the goal of the VFS support is. To my mind, it doesn't make sense that a file is placed in the RealFS in the first place if a VFS is used, but this is quite entrenched in the current design and difficult to change. In which case, I would argue that parsing a file with a preamble and a VFS should not give a cryptic error without an explicit overlay, but rather "just work". But I agree my current patch is a little crude in that it allows access to the entire RealFS via the VFS.
This patch is obsolete. While waiting over a year for a review, somebody else came across the same fix (for a different manifestation of the same bug) in D27810 and managed to get it through. I think my test case and supporting changes are still useful, however, so I've submitted that separately in D37474.
Sep 1 2017
The IsValid = true line coincidentally fixes a tangentially related bug -- see D20338 (in which I tried to introduce the same fix almost a year earlier, but nobody accepted the review). I guess I have to maintain the test for that case out of tree, though I could try to upstream it again (however, given that it's now passing with your change, I don't have much hope of it being accepted).
Jan 3 2017
Thanks, I'll check these out! Btw, I noticed that the clang-format tests are non-Windows due to path assumptions. Is this a lost cause, or just something no one's bothered to look into yet?
No one's bothered looking into it yet.
Nov 15 2016
Oct 6 2016
I'll commit this in a few days, when I have some time to reintegrate it to the trunk. Thanks!
Aug 18 2016
Aug 2 2016
Anyone have time to check this out this week?
It's a one-line fix, includes a test, and is for a fairly important bug :-)
Jul 25 2016
Hmm, I don't think this is easily testable at all. We happened to see it on our (out-of-tree) big-endian architecture when debugging remotely on Windows, but only for variables backed by registers. I was thus able to trace the problem back to this code, fix it, and verify it locally. But I can't reproduce this for other architectures because they likely don't follow the same code path (plus I don't have the hardware/simulator setup required). And trying to unit test this would be... quite a challenge, I feel, considering all the layers involved.
Jul 20 2016
@spyffe, do you have a minute today?
Jul 5 2016
Sorry to bug you again, but @spyffe, could you have a look when you have a sec?
Jun 28 2016
@spyffe, do you have a minute to look this over?
Anyone have a few minutes to look at this?
Jun 16 2016
Jun 14 2016
Thanks everyone :-)
Jun 13 2016
Jun 9 2016
Here's the final fix (it's the line in FileManager.cpp, plus a test).
This is a fairly important bug for anyone hosting clang as a library (e.g. IDEs).
Can someone have a look at this patch when they have a free moment?
Can someone have a look at this now that there's a test?
Jun 3 2016
It took some modifications to the ASTUnit to support a virtual file system with a PCH parse/reparse (preliminary VFS support had already been added in rL249410 but it did not support initial parses using PCHs, nor reparses), but I was finally able to write a test that checks that the reparse actually uses the PCH with my fix, and rejects the PCH (rereading everything and failing the test) without it.
Jun 1 2016
May 30 2016
May 26 2016
Thanks @bruno, I'll have a look at using a VFS for the test.
May 24 2016
I'm not sure how to test this (originally I found this bug by stepping through with a debugger) -- is there a way to determine if an ASTUnit used a PCH for the preamble or not? I'd call the getMainBufferWithPrecompiledPreamble method manually but it's private.
May 18 2016
I've seen rare cases where parses using the PCH files were yielding completely invalid data, almost as if they were corrupted. I wonder now if this could be the cause. (We're on Windows too.)
May 17 2016
May 16 2016
This version of the patch takes into account potential changes between filenames and their unique IDs between parses.