Page MenuHomePhabricator

ilya-biryukov (Ilya Biryukov)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 6 2017, 1:42 AM (88 w, 6 d)

Recent Activity

Yesterday

ilya-biryukov accepted D55820: [AST] Unify the code paths of traversing lambda expressions..

LGTM. The new code looks simpler and it's arguably simpler for the clients, since they'll have a consistent set of callbacks independent of the actual view into the lambda expression.

Tue, Dec 18, 7:23 AM
ilya-biryukov accepted D55437: [Index] Index declarations in lambda expression..

LGTM

Tue, Dec 18, 6:51 AM
ilya-biryukov added inline comments to D55820: [AST] Unify the code paths of traversing lambda expressions..
Tue, Dec 18, 6:46 AM
ilya-biryukov added a comment to D55818: [clangd] Unify path canonicalizations in the codebase.

The change mostly, the only important comment from me is about toURI. I believe it would actually have an observable effect in the real world and I'm not sure things won't break after it.
Could we avoid changing it and focus only on tweaking the "canonical" path for the FileEntries in this change? Those look like a no-op in most practical cases.

Tue, Dec 18, 6:27 AM
ilya-biryukov added a comment to D54945: This commit adds a chapter about clang-tidy integrations.

Thanks for the update! We still need to solve the technicality here: could you please upload the diff with full context? See LLVM docs for an example on how this can be done with various version controls (the -U99999 args are important, they are the ones that produce diffs with enough lines around it)

Tue, Dec 18, 3:46 AM · Restricted Project
ilya-biryukov created D55815: DO NOT SUBMIT. [clangd] A helper Mutex class that reports contention stats.
Tue, Dec 18, 2:30 AM

Mon, Dec 17

ilya-biryukov added a comment to D55437: [Index] Index declarations in lambda expression..

Update after investigating with @hokein offline: the RecursiveASTVisitor has custom code to visit lambda parameters and return type, but it fallback to visiting typelocs when both parameters and a return type are specified. Unfortunately this fallback does not work when shouldWalkTypeLocs() is set to false, which is the case for our visitor out here.
It seems reasonable to always visit parameters and return type, rather than relying on traversing the full type-loc of the lamda's function type.

Mon, Dec 17, 9:30 AM
ilya-biryukov added a comment to D55224: [clangd] Introduce loading of shards within auto-index.

Probably the most important suggestion from my side is trying to clearly separate the enqueue calls (which actually schedule rebuilds of TUs) using clang and the loadShards function.
The index loading should be fast, so I assume we won't win much latency by scheduling the indexing actions in the middle of the shard-loading.
OTOH, separating those two concerns should make the code way more readable (I think, at least).

Mon, Dec 17, 6:26 AM
ilya-biryukov accepted D55417: [clangd] Change disk-backed storage to be atomic.

LGTM with a few nits

Mon, Dec 17, 4:28 AM
ilya-biryukov accepted D55315: [clangd] Only reduce priority of a thread for indexing..

LGTM

Mon, Dec 17, 4:20 AM
ilya-biryukov added a comment to D55363: [clangd] Expose FileStatus in LSP..

Good point, we should definitely state it's a clangd-specific extension. We could come up with a naming scheme for our extensions. I would propose something like clangd:$methodName, i.e. the current extensions would be clangd:textDocument/fileStatus.
WDYT?

Mon, Dec 17, 4:12 AM

Thu, Dec 13

ilya-biryukov added inline comments to D55315: [clangd] Only reduce priority of a thread for indexing..
Thu, Dec 13, 9:29 AM
ilya-biryukov updated the diff for D55648: [CodeComplete] Fill preferred type on binary expressions.
  • Update the test after changes
Thu, Dec 13, 8:05 AM
ilya-biryukov added a comment to D55648: [CodeComplete] Fill preferred type on binary expressions.

Thanks for the review!

Thu, Dec 13, 8:04 AM
ilya-biryukov added inline comments to D55315: [clangd] Only reduce priority of a thread for indexing..
Thu, Dec 13, 7:57 AM
ilya-biryukov updated the diff for D55648: [CodeComplete] Fill preferred type on binary expressions.
  • Address review comments
Thu, Dec 13, 7:53 AM
ilya-biryukov accepted D55649: [clangd] Enable cross-namespace completions by default in clangd.

LGTM. When we know everyone is happy with the new behaivour, we can remove the flag altogether.

Thu, Dec 13, 7:28 AM
ilya-biryukov added a comment to D55437: [Index] Index declarations in lambda expression..

The RecursiveASTVisitor seems to have the code that properly traverses parameters and return types of lambdas. Any ideas why it's not getting called here?

Thu, Dec 13, 6:30 AM
ilya-biryukov added a comment to D55363: [clangd] Expose FileStatus in LSP..

Sorry if I missed any important design discussions here, but wanted to clear up what information we are trying to convey to the user with the status messages?
E.g. why seeing "building preamble", "building file" or "queued" in the status bar can be useful to the user? Those messages mention internals of clangd, I'm not sure how someone unfamiliar with internals of clangd should interpret this information.

Thu, Dec 13, 5:58 AM
ilya-biryukov added inline comments to D55417: [clangd] Change disk-backed storage to be atomic.
Thu, Dec 13, 3:39 AM
ilya-biryukov added inline comments to D55315: [clangd] Only reduce priority of a thread for indexing..
Thu, Dec 13, 3:34 AM
ilya-biryukov created D55648: [CodeComplete] Fill preferred type on binary expressions.
Thu, Dec 13, 3:24 AM
ilya-biryukov added a comment to D55455: Remove stat cache chaining as it's no longer needed after PTH support has been removed.

Yay! Anything that makes FileManager and friends simpler LG

Thu, Dec 13, 2:59 AM
ilya-biryukov accepted D55191: [clangd] Refine the way of checking a declaration is referenced by the written code..

LGTM. Many thanks for the fix, really important to get those cases right.

Thu, Dec 13, 2:20 AM
ilya-biryukov added a comment to D48116: [libclang] Allow skipping warnings from all included files.
In D48116#1322878, @nik wrote:

Have you considered doing the same filtering in ASTUnit's StoredDiagnosticConsumer? It should not be more difficult and allows to avoid changing the clang's diagnostic interfaces. That's what we do in clangd.

Hmm, it's a bit strange that StoredDiagnosticConsumer should also start to filter things.

Thu, Dec 13, 2:16 AM
ilya-biryukov added inline comments to D41005: Reuse preamble even if an unsaved file does not exist.
Thu, Dec 13, 1:39 AM
ilya-biryukov accepted D55359: [clangd] Avoid emitting Queued status when we are able to acquire the Barrier..

LGTM

Thu, Dec 13, 1:21 AM

Fri, Dec 7

ilya-biryukov created D55431: [CodeComplete] Set preferred type to bool on conditions.
Fri, Dec 7, 6:18 AM
ilya-biryukov added inline comments to D55359: [clangd] Avoid emitting Queued status when we are able to acquire the Barrier..
Fri, Dec 7, 6:16 AM
ilya-biryukov added inline comments to D53866: [Preamble] Stop generating preamble for circular #includes.
Fri, Dec 7, 6:09 AM
ilya-biryukov added inline comments to D55315: [clangd] Only reduce priority of a thread for indexing..
Fri, Dec 7, 3:50 AM
ilya-biryukov added a comment to D55417: [clangd] Change disk-backed storage to be atomic.

NIT: I believe we're missing a hyphen in the change description, i.e. it should be disk-backed.

Fri, Dec 7, 3:13 AM

Thu, Dec 6

ilya-biryukov added a comment to D54945: This commit adds a chapter about clang-tidy integrations.

Maybe try to use Arcanist for uploading diffs?
The web interface can be confusing at times, Arcanist is more reliable (LLVM docs have pointers to the Arcanist documentation to get started)

Thu, Dec 6, 6:18 AM · Restricted Project

Wed, Dec 5

ilya-biryukov added a comment to D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries.

Ah, sorry, I incorrectly checked that the old revision landed without problems. Have no data on whether the new revision breaks anything, will have to wait for the input from the US timezone buildcop

Wed, Dec 5, 11:03 AM · debug-info
ilya-biryukov added inline comments to D55315: [clangd] Only reduce priority of a thread for indexing..
Wed, Dec 5, 11:00 AM
ilya-biryukov added inline comments to D53866: [Preamble] Stop generating preamble for circular #includes.
Wed, Dec 5, 10:55 AM
ilya-biryukov added inline comments to D55250: [clangd] Enhance macro hover to see full definition.
Wed, Dec 5, 10:40 AM
ilya-biryukov added inline comments to D51183: [clang-query] Continue if compilation command not found for some files.
Wed, Dec 5, 10:24 AM
ilya-biryukov added a comment to D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries.

It looks like the integrate went smoothly with the new change, thanks for fixing it!

Wed, Dec 5, 10:20 AM · debug-info
ilya-biryukov accepted D54796: [clangd] C++ API for emitting file status.

LGTM, still have a few NITs and suggestions (see the comment about emitting "queued" on waiting for diagnostics and not emitting queued if the lock was acquired from the first try), but they don't look like something that should block this change.

Wed, Dec 5, 10:12 AM
ilya-biryukov created D55331: [CodeComplete] Fix assertion failure.
Wed, Dec 5, 8:31 AM
ilya-biryukov added a comment to D55260: [CodeComplete] Fix a crash in access checks of inner classes.

I believe also we need another test case where Cls and NamingClass are different.

Wed, Dec 5, 7:24 AM
ilya-biryukov updated the diff for D55260: [CodeComplete] Fix a crash in access checks of inner classes.
  • Add a newline to the end of the test file
Wed, Dec 5, 7:23 AM
ilya-biryukov updated the diff for D55260: [CodeComplete] Fix a crash in access checks of inner classes.
  • Reformat
Wed, Dec 5, 7:22 AM
ilya-biryukov updated the diff for D55260: [CodeComplete] Fix a crash in access checks of inner classes.
  • Make sure we still run ObjC access checks.
  • Add a test with a different NamingClass and BaseType.
  • Fix IsSimplyAccessible to work in this case.
Wed, Dec 5, 7:20 AM
ilya-biryukov added a comment to D54630: Move detection of libc++ include dirs to Driver on MacOS.

D55322 is a review for release notes

Wed, Dec 5, 6:52 AM
ilya-biryukov created D55322: Mention changes to libc++ include dir lookup in release notes..
Wed, Dec 5, 6:51 AM
ilya-biryukov added inline comments to D55315: [clangd] Only reduce priority of a thread for indexing..
Wed, Dec 5, 6:13 AM
ilya-biryukov added a comment to D54872: [clangd] Add ability to not use -resource-dir at all.

It doesn't seem like there is any difference in how -resource-dir and /imsvc are handled: they are all added as -internal-isystem

Thanks for digging this code up.
I'm a bit confused now, will put up a few clarifying questions to make sure I understand the probem properly.
Does clang-cl work correctly the arguments from compile_commands.json? Does it fail if we add the -resource-dir, similar to how it was done for clangd? The compile-commands.json seems to be generated for the MSVC or hand-crafted, I wonder if it may be a difference between the clang frontend and msvc (unlikely, but just to be sure).
The compile_commands.json does not even mention the -resource-dir, I wonder why would adding it break anything? The only thing that comes to mind is some of the microsoft headers being named the same as the clang headers, but having more stuff in them (the uintptr_t that's missing). Is that the case?

Wed, Dec 5, 5:13 AM
ilya-biryukov accepted D55312: [clangd] Fix a typo in TUSchedulerTests.

LGTM

Wed, Dec 5, 5:04 AM

Tue, Dec 4

ilya-biryukov accepted D51183: [clang-query] Continue if compilation command not found for some files.
Tue, Dec 4, 12:13 PM
ilya-biryukov added a comment to D51183: [clang-query] Continue if compilation command not found for some files.

LGTM with a few NITs.
Also consider adding a test for this.

Tue, Dec 4, 12:10 PM
ilya-biryukov updated the diff for D55260: [CodeComplete] Fix a crash in access checks of inner classes.
  • Actually use the computed NamingClass
Tue, Dec 4, 3:33 AM
ilya-biryukov created D55260: [CodeComplete] Fix a crash in access checks of inner classes.
Tue, Dec 4, 2:38 AM
ilya-biryukov accepted D55062: [clangd] Partition include graph on auto-index..

LGTM

Tue, Dec 4, 12:57 AM
ilya-biryukov added inline comments to D53866: [Preamble] Stop generating preamble for circular #includes.
Tue, Dec 4, 12:53 AM

Mon, Dec 3

ilya-biryukov added a comment to D54796: [clangd] C++ API for emitting file status.

Sorry for the delays. Not sure about emitting Queued on the main thread, see the corresponding comment. Otherwise looks very good.

Mon, Dec 3, 7:58 AM
ilya-biryukov added inline comments to D41005: Reuse preamble even if an unsaved file does not exist.
Mon, Dec 3, 7:34 AM
ilya-biryukov added a comment to D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap.

I don't think removing the flag is a good idea since I can easily assume cases when user wants mmap and is ready to encounter locks. In our case it can be an IDE option which behavior to choose.

Would that option be useful if changing it forces the files user is editing in the IDE to be indefinitely locked and stops them from saving the files?
Anyway, even if you feel this behavior should be configurable, that's totally fine - I was referring to removing the flag from the vfs::FileSystem interface, possibly replacing it with a filesystem implementation that would handle the same thing at a lower level.
This flag is not useful to any of the filesystem implementation, except RealFileSystem, which actually has to deal with opening OS files.

Mon, Dec 3, 7:22 AM
ilya-biryukov added a comment to D55062: [clangd] Partition include graph on auto-index..

Everything looks good, just a single important request about testing the included files do not have any edges in the resulting graph

Mon, Dec 3, 7:08 AM
ilya-biryukov added a comment to D54630: Move detection of libc++ include dirs to Driver on MacOS.

So far everything looks fine, but I have to rerun a couple of things due to infrastructural issues. I should have the final results next Monday.

Mon, Dec 3, 6:47 AM
ilya-biryukov updated the diff for D55139: [clangd] Avoid memory-mapping files on Windows.
  • s/VolatileFSProvider/VolatileFileSystem
Mon, Dec 3, 5:53 AM
ilya-biryukov added inline comments to D55139: [clangd] Avoid memory-mapping files on Windows.
Mon, Dec 3, 5:50 AM
ilya-biryukov updated the diff for D55139: [clangd] Avoid memory-mapping files on Windows.
  • Keep using memory-mapped files for PCHs
  • Move once
Mon, Dec 3, 5:50 AM
ilya-biryukov added a comment to D55191: [clangd] Refine the way of checking a declaration is referenced by the written code..

The idea of using the AST-based approach here was really nice, it was less expensive and seemed to clearly look at the semantic.
I wonder if there's a way to keep it on the AST level, without looking at the source locations.

Mon, Dec 3, 5:28 AM
ilya-biryukov added a reviewer for D55185: [Clangd] Index main-file symbols (bug 39761): ilya-biryukov.

Thanks for taking a look at this.

Mon, Dec 3, 4:57 AM
ilya-biryukov added a comment to D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap.

Ok, no global option.
Why not placing your VolatileFSProvider in clang so that libclang could you it too?

Mon, Dec 3, 3:48 AM
ilya-biryukov added a comment to D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap.

It seems there are still cases where memory-mapping should be preferred, even on Windows, specifically:

  • PCH files produced by libclang: they are huge, loading them in memory is wasteful and they can clearly be used
  • (maybe?) PCM (precompiled module) files: they are huge, however can be produced by the user's buildsystem. Locking them might be bad if we don't control the buildsystem, but the memory savings are attractive.
  • other binary input files?
Mon, Dec 3, 3:09 AM
ilya-biryukov planned changes to D55139: [clangd] Avoid memory-mapping files on Windows.

We need to keep mmapping the PCH files.

Mon, Dec 3, 2:45 AM
ilya-biryukov added inline comments to D55124: [CodeComplete] Cleanup access checking in code completion.
Mon, Dec 3, 2:18 AM
ilya-biryukov updated the diff for D55124: [CodeComplete] Cleanup access checking in code completion.
  • Do not introduce a new local var, reuse existing
Mon, Dec 3, 2:18 AM

Fri, Nov 30

ilya-biryukov added a comment to D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap.

We can work around it by reading the whole file, but it looks like a bug in QtCreator to me. We have the file mapped, but maybe when they open the file to save it, *they* are opening the file without FILE_SHARE_READ.

I haven't tried QtCreator, but this also happens in VSCode and Vim. I suspect other editors are also affected, but I haven't checked.
I looked at the logs in Process Monitor and I think they do set the share flags properly (~95% certain, will still have to double-check). Even if they do it wrong, it will take time to fix this and I don't think we can afford breaking users with existing editors in our tooling.

Fri, Nov 30, 12:03 PM
ilya-biryukov created D55139: [clangd] Avoid memory-mapping files on Windows.
Fri, Nov 30, 11:50 AM
ilya-biryukov added a comment to D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap.

Original patch description says this:

There are reported cases of non-system files being locked by libclang on Windows (and likely by other clients as well)

What is the nature of the reports? What operation is attempted on the files and fails due to locking? And which application is it that's failing and in what way?

https://bugreports.qt.io/browse/QTCREATORBUG-15449 was mentioned previously in the thread.

Fri, Nov 30, 11:22 AM
ilya-biryukov added a comment to D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap.

Passing-by thought, feel free to ignore: this seems to so far only affect windows only?
So the fix shouldn't probably pessimize all other arches? (and maybe even non-clangd)

Fri, Nov 30, 9:57 AM
ilya-biryukov added inline comments to D54952: [clangd] DO NOT SUBMIT. Draft interfaces for build system integration..
Fri, Nov 30, 9:43 AM
ilya-biryukov updated the diff for D55124: [CodeComplete] Cleanup access checking in code completion.
  • Fix a comment
Fri, Nov 30, 9:33 AM
ilya-biryukov updated the diff for D55124: [CodeComplete] Cleanup access checking in code completion.
  • Add missed field init
  • Also fix access checks for member access with qualifiers
Fri, Nov 30, 9:30 AM
ilya-biryukov added inline comments to D55062: [clangd] Partition include graph on auto-index..
Fri, Nov 30, 9:18 AM
ilya-biryukov added a comment to D54630: Move detection of libc++ include dirs to Driver on MacOS.

@arphaman, did you have a chance to run the tests? There's not rush, just wanted to know whether we have any data at all at how

Fri, Nov 30, 9:14 AM
ilya-biryukov added a reviewer for D55124: [CodeComplete] Cleanup access checking in code completion: kadircet.
Fri, Nov 30, 8:55 AM
ilya-biryukov accepted D54999: [clangd] Populate include graph during static indexing action..

Thank, LGTM. A few NITs.

Fri, Nov 30, 7:50 AM
ilya-biryukov accepted D55054: [clang] Fill RealPathName for virtual files..

LGTM and a few nits

Fri, Nov 30, 7:42 AM
ilya-biryukov created D55124: [CodeComplete] Cleanup access checking in code completion.
Fri, Nov 30, 7:19 AM
ilya-biryukov added inline comments to D54796: [clangd] C++ API for emitting file status.
Fri, Nov 30, 5:49 AM
ilya-biryukov added a comment to D54796: [clangd] C++ API for emitting file status.

@ilya-biryukov, I hope the current patch is not too big for you to review, happy to chat offline if you want (sam and I had a lot of discussions before he is OOO).

Picking it up. Mostly NITs from my side and a few questions to better understand the design. Also happy to chat offline.

Fri, Nov 30, 3:09 AM
ilya-biryukov added inline comments to D55062: [clangd] Partition include graph on auto-index..
Fri, Nov 30, 2:45 AM
ilya-biryukov added inline comments to D55054: [clang] Fill RealPathName for virtual files..
Fri, Nov 30, 2:30 AM
ilya-biryukov added inline comments to D54999: [clangd] Populate include graph during static indexing action..
Fri, Nov 30, 2:15 AM

Thu, Nov 29

ilya-biryukov added inline comments to D54999: [clangd] Populate include graph during static indexing action..
Thu, Nov 29, 9:55 AM
ilya-biryukov added inline comments to D54999: [clangd] Populate include graph during static indexing action..
Thu, Nov 29, 8:15 AM
ilya-biryukov added a comment to D55052: Fix junk output in clangd vscode plugin.

For cancellation errors, it might be reasonable, but what if other unrecoverable errors in clangd (e.g. clangd crashes), in these cases, users don't know what happens, and would still expect IDE features (like code completion) to work...

Yeah, there are still modes where clangd behaves badly there. Note that the output from that pane is rarely useful anyway as clangd keeps producing errors about accessing non-open files, which would confuse people even more.
Overall we never designed this output to be readable by the users, so it's not really useful anyway. There are ways to communicate with the users in the protocol, we should be using them instead to be more user-friendly

Thu, Nov 29, 7:20 AM · Restricted Project
ilya-biryukov added inline comments to D55052: Fix junk output in clangd vscode plugin.
Thu, Nov 29, 6:33 AM · Restricted Project
ilya-biryukov added inline comments to D55052: Fix junk output in clangd vscode plugin.
Thu, Nov 29, 6:32 AM · Restricted Project
ilya-biryukov added a comment to D55052: Fix junk output in clangd vscode plugin.

+1 to the change, this is annoying for me too.

Thu, Nov 29, 6:30 AM · Restricted Project

Wed, Nov 28

ilya-biryukov added inline comments to D54999: [clangd] Populate include graph during static indexing action..
Wed, Nov 28, 9:31 AM
ilya-biryukov added a comment to D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap.

My first thought would be that it depends on the flags to CreateFile moreso (and perhaps entirely) rather than the flags to MapViewOfFile or CreateFileMapping. Specifically, the FILE_SHARE_XXX flags are the most relevant here.

That was my initial thought as well and it worked fine on Windows Server 2012. Will have to do it again to double-check, though, not sure I can dig up that code.

Wed, Nov 28, 9:19 AM
ilya-biryukov added inline comments to D54999: [clangd] Populate include graph during static indexing action..
Wed, Nov 28, 7:56 AM
ilya-biryukov added a comment to D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap.

"could we figure out the exact cause of the issue?"
And this review was not meant to proceed immediately but rather state the intention and get some feedback because I still don't know answers to the questions 1 and 2 (did somebody else investigate that?)

I tried calling Windows APIs that LLVM uses with multiple combinations of flags and couldn't make it lock the file. But maybe I didn't arrive at the right combination of flags or was trying on a different version.

Wed, Nov 28, 7:40 AM
ilya-biryukov updated subscribers of D54995: [MemoryBuffer] Add the setter to be able to force disabled mmap.

cross-posting @zturner's comment from the mailing thread for the record:

I’m afraid this is going to be too severe a performance regression if we change the default. So I don’t think this should be an LLVM-wide default

Wed, Nov 28, 6:39 AM