Incorporate DILLexer changes from upstream commit. #18

cmtice · 2025-02-04T20:10:17Z

Upstream PR: llvm/llvm-project#123521

Note: This test won't compile or run until the other DILLexer changes are committed (upcoming PR).

cmtice · 2025-02-04T20:12:12Z

It looks like I can create PRs for our collaboration work. :-)

kuilpd · 2025-02-05T17:46:14Z

Should we merge #5 first and then put these tests in lldb/unittests/DIL/ as well?

cmtice · 2025-02-05T17:57:35Z

Should we merge #5 first and then put these tests in lldb/unittests/DIL/ as well?

Yes, that makes sense. :-)

Also update all the other DIL files to use the updated Token & Lexer fields & methods appropriately. Update DILGetValueForVariableExpressionPath to create the lexer & pass it to the parser.

cmtice · 2025-02-06T05:15:16Z

Whoops! I meant to create a separate PR with all the rest of my lexer/token changes (from my LLDB upstream PR), but I seem to have pushed all the changes into this PR. Is that ok?

kuilpd · 2025-02-06T09:38:57Z

Yeah, might as well, just need to change the PR title to reflect this :)

cmtice · 2025-02-06T15:27:22Z

Yeah, might as well, just need to change the PR title to reflect this :)

Done.

kuilpd · 2025-02-06T15:50:55Z

There are some merge conflicts due to 21d4384, could you rebase this branch on dil-main first?

cmtice · 2025-02-06T19:15:10Z

There are some merge conflicts due to 21d4384, could you rebase this branch on dil-main first?

Done.

kuilpd · 2025-02-06T19:55:10Z

Now that #5 is merged, let's put the lexer unit tests in lldb/unittests/DIL/, can merge this PR after.

cmtice · 2025-02-06T21:25:18Z

Hm...I'm seeing some new unittest failures now (possibly due to my lexer changes?). I need to look into those...

kuilpd · 2025-02-08T14:43:46Z

Hm...I'm seeing some new unittest failures now (possibly due to my lexer changes?). I need to look into those...

Also some XFailed tests could become fixed because the changes, so they result in an error now, try removing XFail(...) around the matcher to see if it fixes the test.

cmtice · 2025-02-09T00:46:27Z

This is passing all the unittests except EvalTest.TestCxxStaticCast. It's failing that with:

/usr/local/google3/cmtice/access-softek-dil/lldb/unittests/DIL/DILTests.cpp:1573: Failure
Value of: Eval("static_cast<nullptr_t>((int)0)")
Expected: evaluates with an error: 'static_cast from 'int' to 'nullptr_t' (canonically referred to as 'std::nullptr_t') is not allowed'
Actual: { DIL: NULL, lldb: NULL }, error:
expr:1:13: unknown type name 'nullptr_t'
static_cast<nullptr_t>((int)0)
^

/usr/local/google3/cmtice/access-softek-dil/lldb/unittests/DIL/DILTests.cpp:1577: Failure
Value of: Eval("static_cast<nullptr_t>((void*)0)")
Expected: evaluates with an error: 'static_cast from 'void ' to 'nullptr_t' (canonically referred to as 'std::nullptr_t') is not allowed'
Actual: { DIL: NULL, lldb: NULL }, error:
expr:1:13: unknown type name 'nullptr_t'
static_cast<nullptr_t>((void)0)
^

I'm not sure why it's not recognizing nullptr_t as a valid type name, nor how to fix that. Do you have any ideas? If not, I'll dive into this a bit more...

cmtice · 2025-02-09T00:47:40Z

It might be worth merging this into the main branch now, and we can continue to look into the test failure after that. Just a thought.

kuilpd · 2025-02-10T16:47:07Z

I'm not sure why it's not recognizing nullptr_t as a valid type name, nor how to fix that. Do you have any ideas? If not, I'll dive into this a bit more...

This is the same error I skipped with the last commit in unit tests PR. It doesn't fail on my system, so it's hard to debug. I think we can merge this PR, since I'm pretty sure the error has nothing to do with DIL, just with nullptr_t not being included in the test binary.

I have a couple of ideas though:

Add #include <cstddef> to test_binary.cc (and maybe DILTests.cpp as well). This is where nullptr_t is defined, maybe this will fix the problem.
I didn't test lldb/unittests/DIL/Inputs/CMakeLists.txt on any other system, could you check if generated variables LIBCXX_LIBRARY_DIR and LIBCXX_GENERATED_INCLUDE_DIR are actually pointing to in-tree files on your system?
Are you building this on an Apple device? I saw some mentions online that LLDB (or clang in general) could have problems with including (finding) C headers there. In this case, no idea what to do :)

…ils.

cmtice · 2025-02-11T05:32:57Z

I'm not sure why it's not recognizing nullptr_t as a valid type name, nor how to fix that. Do you have any ideas? If not, I'll dive into this a bit more...

This is the same error I skipped with the last commit in unit tests PR. It doesn't fail on my system, so it's hard to debug. I think we can merge this PR, since I'm pretty sure the error has nothing to do with DIL, just with nullptr_t not being included in the test binary.

I have a couple of ideas though:

Add #include <cstddef> to test_binary.cc (and maybe DILTests.cpp as well). This is where nullptr_t is defined, maybe this will fix the problem.

I didn't test lldb/unittests/DIL/Inputs/CMakeLists.txt on any other system, could you check if generated variables LIBCXX_LIBRARY_DIR and LIBCXX_GENERATED_INCLUDE_DIR are actually pointing to in-tree files on your system?

Are you building this on an Apple device? I saw some mentions online that LLDB (or clang in general) could have problems with including (finding) C headers there. In this case, no idea what to do :)

I tried including <cstddef> in test_binary.cc, and DILTests.cpp, but that didn't help. I verified LIBCXX_LIBRARY_DIR and LIBCXX_GENERATED_INCLUDE_DIR were valid. I'm not building on an Apple device; I'm building on Linux.

I debugged the code and found a bug in ResolveTypesByName (In DILAST.cpp): It was finding. a partial match between "std::__1::nullptr_t" and "nullptr_t", but it wasn't using/returning the partial match. I've fixed this (and uploaded the fix to this PR). I'm still getting a failure, but now the failure is just a mismatch in the error messages:

[ RUN ] EvalTest.TestCxxStaticCast
/usr/local/google3/cmtice/access-softek-dil/lldb/unittests/DIL/DILTests.cpp:1573: Failure
Value of: Eval("static_cast<nullptr_t>((int)0)")
Expected: evaluates with an error: 'static_cast from 'int' to 'nullptr_t' (canonically referred to as 'std::nullptr_t') is not allowed'
Actual: { DIL: NULL, lldb: NULL }, error:
expr:1:1: static_cast from 'int' to 'std::__1::nullptr_t' (canonically referred to as 'std::nullptr_t') is not allowed
static_cast<nullptr_t>((int)0)
^

/usr/local/google3/cmtice/access-softek-dil/lldb/unittests/DIL/DILTests.cpp:1577: Failure
Value of: Eval("static_cast<nullptr_t>((void*)0)")
Expected: evaluates with an error: 'static_cast from 'void *' to 'nullptr_t' (canonically referred to as 'std::nullptr_t') is not allowed'
Actual: { DIL: NULL, lldb: NULL }, error:
expr:1:1: static_cast from 'void ' to 'std::__1::nullptr_t' (canonically referred to as 'std::nullptr_t') is not allowed
static_cast<nullptr_t>((void)0)
^

/usr/local/google3/cmtice/access-softek-dil/lldb/unittests/DIL/DILTests.cpp:1604: Skipped
Segfault when retrieving result value in the matcher

(I do worry a bit about that last line, about the Segfault in the matcher...)

kuilpd · 2025-02-11T19:40:23Z

I debugged the code and found a bug in ResolveTypesByName (In DILAST.cpp): It was finding. a partial match between "std::__1::nullptr_t" and "nullptr_t", but it wasn't using/returning the partial match. I've fixed this (and uploaded the fix to this PR).

Good catch, and with this change I realized that the command I wrote to build the test binary is linking it with the in-tree libc++ wrong, so we're still getting different libc++ versions linked. I will make another PR fixing that, we can merge this PR now.

/usr/local/google3/cmtice/access-softek-dil/lldb/unittests/DIL/DILTests.cpp:1604: Skipped Segfault when retrieving result value in the matcher

(I do worry a bit about that last line, about the Segfault in the matcher...)

This is just a message I wrote as a reminder why the following tests after DILTests.cpp:1604 are skipped, I'll look into it eventually.

Add DILLexerTests.cpp unit test.

ed525b0

Note: This test won't compile or run until the other DILLexer changes are committed (upcoming PR).

cmtice requested review from asl and kuilpd February 4, 2025 20:11

Update DILLexer and DILToken classes to match upstream PR.

c5fd8fe

Also update all the other DIL files to use the updated Token & Lexer fields & methods appropriately. Update DILGetValueForVariableExpressionPath to create the lexer & pass it to the parser.

cmtice changed the title ~~Add DILLexerTests.cpp unit test.~~ Incorporate DILLexer changes from upstream commit. Feb 6, 2025

Merge remote-tracking branch 'origin/dil-main' into update-lexer

66669fd

Fix (most) DIL unittest failures

84b394e

Fix ResolveTypeByName to return random partial match if full match fa…

7d454a8

…ils.

cmtice merged commit 77472c7 into dil-main Feb 11, 2025
2 checks passed

Incorporate DILLexer changes from upstream commit. #18

Incorporate DILLexer changes from upstream commit. #18

Uh oh!

Conversation

cmtice commented Feb 4, 2025 • edited by kuilpd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmtice commented Feb 4, 2025

Uh oh!

kuilpd commented Feb 5, 2025

Uh oh!

cmtice commented Feb 5, 2025

Uh oh!

cmtice commented Feb 6, 2025

Uh oh!

kuilpd commented Feb 6, 2025

Uh oh!

cmtice commented Feb 6, 2025

Uh oh!

kuilpd commented Feb 6, 2025

Uh oh!

cmtice commented Feb 6, 2025

Uh oh!

kuilpd commented Feb 6, 2025

Uh oh!

cmtice commented Feb 6, 2025

Uh oh!

kuilpd commented Feb 8, 2025

Uh oh!

cmtice commented Feb 9, 2025

Uh oh!

cmtice commented Feb 9, 2025

Uh oh!

kuilpd commented Feb 10, 2025

Uh oh!

cmtice commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuilpd commented Feb 11, 2025

Uh oh!

Uh oh!

Uh oh!

cmtice commented Feb 4, 2025 •

edited by kuilpd

Loading

cmtice commented Feb 11, 2025 •

edited

Loading