In D88850#2322324, @stella.stamenova wrote:

In D88850#2315053, @aganea wrote:

In D88850#2314943, @stella.stamenova wrote:

In D88850#2312893, @aganea wrote:

@stella.stamenova Strangely, your initial bug description sounds like a deadlock initiated by tar_version.communicate() in https://github.com/llvm/llvm-project/blob/f7941d98091827b8d0b6fdabb731e38c99f44b13/lld/test/lit.cfg.py#L110
Are the lit tests using python 3.6 and not the 2.7 that you seem to have installed as well? In any case, I wonder if setting stderr=open(os.devnull) at lld/test/lit.cfg.py#L108, would fix the issue? Or perhaps just check the 'Threads' tab in Process Explorer on tar.exe, see why it is blocked.

It does, doesn't it! But it's not. If I manually kill tar.exe, then any of the tests that use the git tools time out and fail as well, because all of the tools hang similarly. I attached a debugger to tar.exe after it hung and it is getting an access violation when trying to call GetCommandLineA, so I ran the tests as a different user and they passed. It looks like the tools that come with git can't run successfully under an account that has restricted permissions while the tools that come with GnuWin32 can (also, any other tools we are using in the tests run just fine under the same restricted account, the git tools are the exception). This makes me question whether using the git tools by default is the right choice - the failure was not obvious and not easy to track down. Granted, it is not one that will likely happen on a developer machine, but anyone who is trying to set up automated testing on Windows might (and likely should) be using a restricted account. If we do decide to proceed with using the git tools by default, this should at least be documented and gnuwin32 should be an alternative. Thoughts?

Thanks for investigating! Isn't that related to access rights to the binaries, rather than an issue with the Git tools?

I haven't had time to dig deeper into this, but I don't think it's an issue of access *to* the binaries. The same process that runs the tests, runs git before that and it succeeds. I suppose the git installation could set different permissions on the different paths in the install, but I checked them and they appear to be uniform.

In D84380#2315380, @hans wrote:

To me this seems to have added an extra dimension. Previously, lit would look for the tools on PATH, now it was made to look in two places.

In D84380#2310504, @stella.stamenova wrote:

In D84380#2309885, @stella.stamenova wrote:

In D84380#2309853, @aganea wrote: Maybe if L28 wasn't there, things would work correctly if you already had the required tools in %PATH%? Would you possibly try that please?

I will try it. I'll let you know if it makes a difference.

Yes, that works. We don't end up prepending the lit path and the tests work as before.

That's.... Pretty strange. I was pretty sure I landed it. Feel free to
do this on my behalf. Sorry about that.

I went ahead and committed this. Let's see if I break anything, it's been a long time since I've committed a patch and I haven't committed anything since the git switchover :D

Perhaps now you ;-)

In D84423#2170264, @amccarth wrote:

This looks fine to me. Is there a lit expert/owner who should also review?

I'm not sure the first branch in the new code adds any value.

The first branch is important because lit_tools_path is up to the individual local config to define. In other words, if the user does not specifically write config.lit_tools_path = Foo/Bar then this will be undefined and they'll get strange errors. In all LLVM cases, the configs do this, and this knowledge has been propagated across all LLVM lit configs by way of copy/paste. But it's a silent / undocumented requirement that caused me some confusion until I tracked it down. So it seems better to be resilient in the case of it not being there.

If the current listed reviewers on this patch are not the best people for reviewing this, would one of them please suggest a more appropriate reviewer so we can get some traction on this?

Adding a ping since it's been a week with no additional feedback for the author.

In D70553#1757862, @aaron.ballman wrote:

Can you add a test case for this functionality?

Addressed suggestions from @Eugene.Zelenko

• Updated documentation for this check
• Incorporated additional suggestions from @aaron.ballman
• Fixed an invalid transformation that was generated when binding a member function and the second argument of bind (the object pointer) was a placeholder. Test is added for this case as well.
• Fixed an invalid transformation that was generated when a placeholder index was entirely skipped, as in the call std::bind(add, 0, _2); In this case we need to generate an unused placeholder in the first position of the resulting lambda's parameter list.
• Added a clang-tidy option PermissiveParameterList which appends auto&&... to the end of every lambda's placeholder list. This is necessary in some situations to prevent clang-tidy from applying a fixit that causes the code to no longer compile.

Forgot to remove spurious llvm:: namespace qualifications.

Updated with suggestions from @aaron.ballman

I have a more comprehensive version of this patch that I'll upload separately.

Does this change crash recovery semantics in any meaningful way? Will we still be able to get stack traces on all platforms when the compiler crashes?

Minor cleanup -- moved isFixitSupported logic to its own function.

In Windows you just write a Python script to do this, and this works everywhere, not just on one platform or the other, so bash isn't even necessary and Python is easy to write so I wouldn't really say it's "even harder". If you google for run-clang-format.py you'll find a script that actually branches out and does this in parallel. There's a lot of logic and smarts you could bake into an external tool which can then be used for many different programs, not just clang-format.

Planning to commit this this afternoon unless someone has objections.

In D68736#1702482, @amccarth wrote:

This matches the behavior of cl.

Are you sure? In a bare environment, cl.exe doesn't include any system paths, not even to the standard library. It actually uses the INCLUDE environment variable for those paths. Granted, VCVARSALL sets that (and other environment variables), but it's not innate behavior of cl.exe.

Rebased this onto tip of trunk. I'm *finally* going to submit this.

What's the status of this patch, out of curiosity? It doesn't seem there were any objections to the original idea, just that nobody with ownership over clang-format is still actively participating in the review.

Finally got around to trying to commit this.

In D66431#1636216, @aganea wrote:

In D66431#1636198, @zturner wrote:

Ugh nevermind, you can't because of the binary files. Fineeeeee.

Send the binaries to my email.

Ugh nevermind, you can't because of the binary files. Fineeeeee.

I know it's super lame to request this, but would one of you mind submitting on my behalf? I don't have SVN installed and it seems like git llvm push still requires it (unless there is a more modern workflow that doesn't require SVN anymore). If I find myself submitting more and more patches then I'll bite the bullet.

No objections from me on the wording of the layering portion

I just built against trunk with VS 2019 and ran into this exact issue. llvm-tblgen had no command line options. Are more fixes still in the pipeline or was this thought to be sufficient?

Also remove LLDBTriggerable.py

Add the scheduler back but remove the android builder only.

Rebased

In D59862#1450812, @lanza wrote:

Can't you just write a function that, every time you call it, traces the symbol back to its original compile unit (you can get this from the PdbCompilandSymId), get the CompileUnit item, and ask it for its language? The part that seems unnecessary is the cache.

For a general PDBSymbol? There's a getCompilandId for PDBSymbolFunction and PDBSymbolData which get the compliand from the DIALines they hold. There's a GetPDBCompilandByUID that accepts an arbitrary ID and dyn_cast_or_null's it to a Compiland. I don't see anything else given that a general PDBSymbol doesn't have any access to it's parents.