- User Since
- May 24 2017, 5:35 AM (81 w, 2 d)
Wed, Dec 12
Add standard prologue to the new header
Mon, Dec 10
Fri, Dec 7
We can also add an extra variable for it if you care about build time
I generated the wrong diff. This is the proper one.
Tue, Dec 4
The fact that so many call-sites decide what to do is pretty error-prone.
There was already at least one issue when this flag was not properly set by some of the call-sites.
Mon, Dec 3
Hm. What about another way around? - We have user include paths (-I) and report them to the filesystem. This means that we have specific paths under which nothing can be mmaped and everything else can be. In particular cases we can also report -isystem there. This is quite the same logic as current isVolatile parameter but is set only once for each path.
Ok, no global option.
Why not placing your VolatileFSProvider in clang so that libclang could you it too?
Instead of forcing some default value let's give the client an ability to force disabled mmap if one desires.
Fri, Nov 30
What do you think about the global settable bool in MemoryBuffer in place of the ifdef from https://reviews.llvm.org/D35200 ? In this case the client on Windows can set it and you're safe that any MemoryBuffer call never mmaps.
No success with unmapping. Buffers are hold by PCMCache in Preprocessor (and the same one in ASTReader) and if I clean them then SourceManger crashed sooner or later on some call that gets data from external files.
So far I have no steps to reproduce the lock on user file so I don't currently know if I try something else.
Thu, Nov 29
I'm currently trying out another suggestion - which unmaps memory buffer caches after ASTUnit's Parse or Reparse and is limited to Windows only.
And my aim currently is not only clangd but any other client as well.
According to https://msdn.microsoft.com/en-us/2e9c3174-af48-4fa3-9f6a-fb62b23ed994 - "Unmapping a mapped view of a file invalidates the range occupied by the view in the address space of the process and makes the range available for other allocations".
Also as far as i understand from https://msdn.microsoft.com/en-us/library/ms810613.aspx mapped files can only be edited in other apps as mapped files opened with the same name (OpenFileMapping).
Wed, Nov 28
To clarify a little bit - i did not write my own simple app but used libclang in qt creator to reproduce the lock and track the issue. This time I hope to go further, collect flags used by clang and make a simple example.
I was able to lock files last time with mmap. The Windows API calls are CreateFileMappingW (it's in the Path.inc, line 794) and MapViewOfFile (further down). As far as I remember the second call actually creates the lock and is freed only by UnmapViewOfFile in destructor
According to https://stackoverflow.com/a/7414798 (if it's true) we can't prevent locks.
"could we figure out the exact cause of the issue?"
Hm, probably FileManager can be that adapter since it's in clang
I've realized that this patch covers too much stuff outside of clang and I have no idea how bad is to not memory map it.
I have the reported evidence but didn't yet have time to investigate myself.
This is probably caused by our upgrade to Clang-7 which shows that we can't rely on the aimed fixes like the one I mention (D47460).
VFS is moved to llvm in 8.0, update the diff
Tue, Nov 27
Oct 31 2018
As far as I understand the problem the same thing happens when you are in the header a.h which includes b.h and b.h includes a.h at the same time. So you get this recursion indirectly and very often because that's why include guards are there.
Oct 19 2018
Do you know the better way to accomplish my aim than adding an option to libFormat? For example making a dependent library which serves a little different purpose than libFormat itself or something simpler?
Oct 10 2018
Sep 21 2018
Provide CorrectedBase = Base for C code
You're right actually. The fix is much simpler than I expected :)
Sep 19 2018
I tried that first but did not I find a way just to copy an expression (we basically need the same expr for such case). Do you know how to properly generate a copy of expression or some other way to get the same expression?
Test is added
Sep 7 2018
Sep 5 2018
Aug 28 2018
You are completely right! Thanks!
I did not think that -test-print-type also checks for the pointee.
Aug 26 2018
Aug 23 2018
Let's proceed with this one. I really see that it's going to be useful.
Jul 31 2018
File locking is the first one. Another one comes with "plugin mode" of tidy.
Anyways, if c++ part does not seem relevant I'm fine if we only have libclang part from D48116.
And we already saw, that -isystem is not the best choice for that. And by the way - clang-tidy has this filtering in consumer which does not exist in it's plugin-mode so it spits all system diagnostics all the time...
I already mentioned in the review inherited by this one that this way covers also loaded plugins with different consumers. So let's assume that driver loads clang-tidy and some other plugins and runs over each file in the project.
It makes sense not to include clang-tidy diagnostics coming from headers, at least for third-party headers (you can still run clang over specific headers themselves to get diagnostics for them).
Restore missing tests
Jul 25 2018
I'm mostly fine with your set of patches. Let me double check and we can get it in.
Jul 24 2018
@ilya-biryukov I would name this revision "for test purpose". It works at some extent with the current clangd version and can be used by someone else. But since we had a discussion about YAML being a temporary solution I never planned to commit this revision.
So yes, I totally agree that using a binary format would be ideal for the "index while build' use case.
Jul 13 2018
Jul 11 2018
Jul 6 2018
Jul 4 2018
@ilya-biryukov Sorry. I didn't have time to post comments here.
The usecase that we have is a supportive translation unit for code completion. Probably you use something similar in clangd not to wait for the TU to be reparsed after a change?
The gain from this change is both performance and memory but I don't have measurements under my hand right now. And of course they are only valid for this usecase.
Jul 3 2018
So I have that working on windows with clang-cl (probably broke something else during the porting, need to recheck.
One more commit I have in my working tree is YAML export to be able to use it for clangd. The quality of code is currently not so good so I probably need some extra time to make it ok-ish. But i hope to have it posted to reviews this week (unless I have something urgent)
Jul 1 2018
@nathawes I've already started changing so I can hopefully share my code with you when I fix all undefined symbols :)
Jun 29 2018
Will it work if I replace every block-related code here with llvm::function_ref<>?
I can't really test this one because INDEXSTORE_HAS_BLOCKS is apple-specific and it's required to rewrite all block-related stuff to use that code
Jun 27 2018
But this one misses a way to set this flag for everything except libclang. We can probably change that (Nikolai is in vacation till the end of summer so it's probably my part now) and add some tests outside of Index for it (probably Frontend)
The idea was to ignore everything including notes and remarks so that only errors from system headers are still collected.