- User Since
- Feb 9 2016, 1:33 PM (296 w, 5 d)
Dec 14 2020
Dec 1 2020
I can add a constant.
Oct 13 2020
This fixes https://bugs.llvm.org/show_bug.cgi?id=47743 for me.
Nov 15 2019
Rebased diff on latest upstream.
I've got a rebased diff but Phabricator won't let me attach it to this differential review (too old?). What do I do?
Nov 14 2019
Nov 8 2019
I disagree, ld emitting invalid character warnings is simply a good indication that it was never tested with such an input :-)
ld ignores the BOM, but still links. lld mistakes it for part of the next token and generates an error.
Oct 9 2019
I've committed a fix in rG745e57c5939e. Sorry about that.
Any objections before I commit?
Oct 8 2019
Right, I'll add comments on the pointer declarations.
@aprantl No. A weak pointer would still fix the memory leak, but it's safe to use a raw pointer because we only reference objects which are in the same cluster as the synthetic children front-end itself. The other (leak-free) synthetic front-ends do this as well. We want shared/weak pointers externally, but not within the cluster.
Sep 3 2019
Apr 1 2019
Mar 28 2019
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