• Address builtin redefining (Thanks Michael!)

import-std-module test changes look good to me, thanks for fixing that up.

In D111978#3623374, @MaskRay wrote:

Is this ready to be submitted?

I don't really have the full context here, but I am wondering if we shouldn't somehow take the DW_AT_declaration attribute into account here. It seems like that should give a more definitive answer as to whether we can expect to see a full set of template parameters or not.

Don't have access to a macOS machine that can run the tests, so maybe someone should give this a spin before landing :)

This is beautiful

Thanks, reverting while investigating.

LGTM, thanks! FWIW, if you have fish installed you might wanna check what they are doing on your terminal for its autosuggestions (and we might wanna steal their default, even though I thought this was 'faint').

Just some drive-by comments, but in general I think this would be nice to get fixed.

I'm really sorry @martong , but I no longer work on LLDB (or the ASTImporter) and I'm not really in the loop regarding LLDB development.

Btw, that patch that I referenced hasn't landed yet (it just lacks the tests and probably rebasing), but I'm OOO for an unknown duration so feel free to land it yourself.

LGTM, thanks for fixing this!

In D113498#3131336, @werat wrote:

In D113498#3124525, @teemperor wrote:

I really like the solution, but I think by fixing the CanInterpret you also made the test case no longer reach the actual changed interpreting logic?

So, CanInterpret says "we can't interpret this" (which is correct), but then the interpreter won't run it and your change to ResolveConstantValue isn't actually tested. There is no other test that touches that logic from what I can see. You could try throwing in some other expression at this that tests that new code? Maybe some kind of pointer arithmetic on a variable defined in your expression itself (so it can be constant evaluated).

I think you're right, the interpreter now doesn't get to evaluating the operands of GetElementPtr. However, I've failed to construct an expression that would have a constant expression with getelementptr instruction. So far I've only been able to reproduce it with the example from the test case (which is being rejected during CanInterpret). I've tried expressions like const int x = 1; (int*)100 + (long long)(&x) and similar (also with x being a global variable), but they're are being compiled in a way that getelementptr is not a constant expression anymore.

• Update from git

@labath raised some concerns in IRC about the setup code being run in a potential multithreaded env (which makes sense). Feel free to revert (not sure when/if I'll get around to address the issues)

LGTM, thanks!

Thanks a lot for fixing this, could you point to D103532 as the cause for the regression in the commit message?

I really like the solution, but I think by fixing the CanInterpret you also made the test case no longer reach the actual changed interpreting logic?

LGTM, thanks!

In D113634#3124401, @ljmf00 wrote:

In D113634#3124042, @teemperor wrote:

Are the DWARFASTParserClang changes meant as a step towards making it parse D?

Yes, not only D but any language that currently falls here (https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp#L106-L112). AFAIK, rust have immutable variables too. Since I'm working on a DWARFFASTParser for D this won't affect it in the future, but for languages like Rust that uses the Python API and rely on Clang DWARFParser, this could be beneficial.

In D111409#3124194, @labath wrote:

In D111409#3124075, @teemperor wrote:

Are you asking for dedicated physical resources for running nightly builds?

I don't think any of the current bots have a Java installation so I think it's either that or we get someone with a bot to setup the required Java installation.

I don't have a problem with installing the necessary packages on the bot I manage, but I cannot subscribe to tracking down any failures (or flaky tests!) for this configuration (and, in my experience, any new feature like this is going to have flaky tests). Flaky tests (probably just one) are the reason that lua integration is not enabled on this bot.

Are you asking for dedicated physical resources for running nightly builds?

Are the DWARFASTParserClang changes meant as a step towards making it parse D?

I changed the commit name from "Add support for D programming language" to "[lldb] Add support for demangling D symbols" when landing (which is more accurate).

I guess that's just clang-format ran over the file? If yes, then LGTM

I actually didn't see that the patch deleted the TestCppReferenceToOuterClass test. Could you just add a @skipIf # Crashes or so above its def test... method? The test itself is still valid user code that we shouldn't crash on.

• Clang-format some last minute renames

I'm pretty sure you're trying to solve the same problem as here: D85993

Note that the changed Dump method and the dump method that takes an ExecutionContext are not tested as we can't test those things via unit tests. They can only be tested with an updated DWARF parser on a real executable/process.

LGTM, thanks for this!

0xffffffff still seems to randomly find __NSCFNumber according to Green Dragon (and the same happens locally with an about 10% chance).

LGTM, thanks!

In D112439#3102752, @xujuntwt95329 wrote:

Thanks a lot for your comments, they are very useful and I learned a lot about C++ by talking with you.

I'll address these comments and submit a patch and request your review.

Much appreciated！

Fixed in 58dd658583eec9af24ca1262e1bce9f884d65487

In D112439#3102559, @xujuntwt95329 wrote:

In D112439#3102548, @teemperor wrote:

In D112439#3102533, @xujuntwt95329 wrote:

In D112439#3102506, @teemperor wrote:

In D112439#3098307, @xujuntwt95329 wrote:

Seems that patch can't build by CI because it is based on this patch. In my understanding we need to merge this patch firstly and rebase that NFC patch to let CI work, right?

You can set parent/child revisions in Phabricator that should handle this situation (not sure if the premerge-checks are respecting that, but that's how Phab usually manages unmerged dependencies).

But to be clear, the premerge checks on Phabricator are *not* building or testing LLDB at all. See the CI log:

INFO    projects: {'lldb'}
INFO    all excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 'openmp', 'lldb'}
INFO    effective projects list set()

Thanks a lot for the detailed information!

BTW, I see there are some test failure reported by CI, but I can't reproduce them in my local environment (X86_64 Ubuntu and Windows), is there any docker images to simulate the CI environment?

Can you try a Release + Assert build? I'm looking at the failures at the moment too and it seems to only fail in non-Debug builds.

Sure, I'll try the release + assert build. Thanks again!

In D112439#3102533, @xujuntwt95329 wrote:

In D112439#3102506, @teemperor wrote:

In D112439#3098307, @xujuntwt95329 wrote:

Seems that patch can't build by CI because it is based on this patch. In my understanding we need to merge this patch firstly and rebase that NFC patch to let CI work, right?

You can set parent/child revisions in Phabricator that should handle this situation (not sure if the premerge-checks are respecting that, but that's how Phab usually manages unmerged dependencies).

But to be clear, the premerge checks on Phabricator are *not* building or testing LLDB at all. See the CI log:

INFO    projects: {'lldb'}
INFO    all excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 'openmp', 'lldb'}
INFO    effective projects list set()

Thanks a lot for the detailed information!

BTW, I see there are some test failure reported by CI, but I can't reproduce them in my local environment (X86_64 Ubuntu and Windows), is there any docker images to simulate the CI environment?

In D112439#3098307, @xujuntwt95329 wrote:

Seems that patch can't build by CI because it is based on this patch. In my understanding we need to merge this patch firstly and rebase that NFC patch to let CI work, right?

I think this was just fixed by 48677f58b06cfb8715902173c5bc3d1764d7c8c6

In D112165#3100929, @dblaikie wrote:

In D112165#3100608, @teemperor wrote:

Small note: gmodules test are never run on Linux, so you actually have to run them on macOS (or I think FreeBSD) to know whether the tests work.

Yeah, I'll admit I didn't test this, but seemed consistent with the other changes/cleanup - did this cause any breakage you know of?

Could gmodules be tested on Linux? (I had originally really hoped/tried to encourage gmodules to be implemented in a way that'd be compatible with Split DWARF but it never quite got there, unfortunately... would've made more overlap in functionality/testability/portability, I think)

(I should setup a build environment on my Macbook Pro, but haven't got around to it as yet)

Small note: gmodules test are never run on Linux, so you actually have to run them on macOS (or I think FreeBSD) to know whether the tests work.

LGTM

Thanks!