Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust-gdb/rust-lldb: Trim output of vec/slice/array #30250

Closed
wants to merge 2 commits into from

Conversation

untitaker
Copy link
Contributor

Fix #29467

I have tested this with rust-gdb, but rust-lldb is completely untested.

It prints "cannot access memory" for the example in #29467 instead of hanging
up.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@untitaker untitaker changed the title Trim output of vec/slice/array rust-gdb/rust-lldb: Trim output of vec/slice/array Dec 7, 2015
@alexcrichton
Copy link
Member

r? @michaelwoerister

@michaelwoerister
Copy link
Member

Looks good to me. Let's see if it passes the LLDB tests.

@michaelwoerister
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 7, 2015

📌 Commit 0899f9d has been approved by michaelwoerister

@bors
Copy link
Contributor

bors commented Dec 7, 2015

⌛ Testing commit 0899f9d with merge a6e0bd7...

@bors
Copy link
Contributor

bors commented Dec 7, 2015

💔 Test failed - auto-mac-32-opt

@untitaker
Copy link
Contributor Author

The scoping bugs in the lldb scripts should now be fixed.

@untitaker untitaker force-pushed the max-children branch 2 times, most recently from c02c10e to 95710a1 Compare December 7, 2015 14:03
@untitaker
Copy link
Contributor Author

I'm not sure if there's something wrong with my dev environment, because I can't get the tests to fail locally when reverting the actual fix.

@untitaker untitaker force-pushed the max-children branch 2 times, most recently from f43fc2b to e525b48 Compare December 7, 2015 14:06
@michaelwoerister
Copy link
Member

The test case should not contain the #![omit_gdb_pretty_printer_section] since we actually want to test the GDB pretty printers.

@untitaker untitaker force-pushed the max-children branch 2 times, most recently from f7dc019 to a988191 Compare December 8, 2015 19:19
@untitaker
Copy link
Contributor Author

@michaelwoerister I've removed that attr, please take a look again. It seems that the testcase passes both with and without the fix, but I'm not sure why.

@untitaker
Copy link
Contributor Author

My current suspicion is that ... is interpreted as a wildcard by the testrunner?

@michaelwoerister
Copy link
Member

Only [...] should be interpreted as a wildcard. Let me see if I can reproduce your results.

@untitaker
Copy link
Contributor Author

I see, I've updated the ... to .. though

@michaelwoerister
Copy link
Member

@untitaker OK, so I've looked into LLDB and since we are producing actual strings there, it would be a greater undertaking to do the same as in GDB (and it's not clear that we actually want to do that there).

My proposal would be:

  • Move MAX_CHILDREN into lldb_rust_formatters.py and only use it there.
  • Make the GDB versions return an iterator as @tromey suggested (by just removing the list() call, presumably).
  • Adapt the test cases accordingly.

@untitaker
Copy link
Contributor Author

I'm unsure on how to adapt the tests, and can't get them to fail. I'm thinking about removing them.

@untitaker untitaker force-pushed the max-children branch 3 times, most recently from 2eadf9f to fd4c064 Compare December 13, 2015 17:17
@michaelwoerister
Copy link
Member

You can take the adapted tests from here:
https://github.com/michaelwoerister/rust/blob/pp-huge-vec/src/test/debuginfo/huge-vec.rs
This version worked for me on Linux and Mac OS.

@untitaker
Copy link
Contributor Author

Updated with your test. The first testcase build should fail, the second one pass. Otherwise something is seriously broken.

@untitaker
Copy link
Contributor Author

The relevant tests unexpectedly pass on Travis, but I get stylecheck errors.

@michaelwoerister
Copy link
Member

You need to add the line // ignore-tidy-linelength below // min-lldb-version: 310 to tell the build script that it shouldn't care about line length in this file.

@untitaker
Copy link
Contributor Author

But why do the tests pass without the fix?

@michaelwoerister
Copy link
Member

Where do you see them passing? In the Travis log I only see 'make tidy' running (and failing).

@untitaker
Copy link
Contributor Author

Sorry, yes. Updated PR.

@michaelwoerister
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2016

📌 Commit be44f1e has been approved by michaelwoerister

@bors
Copy link
Contributor

bors commented Jan 12, 2016

⌛ Testing commit be44f1e with merge c50d71a...

@bors
Copy link
Contributor

bors commented Jan 12, 2016

💔 Test failed - auto-win-gnu-64-opt

@michaelwoerister
Copy link
Member

Almost passing. Pretty printers are not enabled on Windows.
Please add the following directives (somewhere near min-lldb-version):

// ignore-windows
// ignore-freebsd
// ignore-android
// min-gdb-version 7.7

Those are the same as in the pretty-std.rs test file, so they should be up-to-date.

@untitaker
Copy link
Contributor Author

I'd like to wait for #30249 such that I can run the gdb/lldb tests in Travis.

@alexcrichton
Copy link
Member

Looks like #30249 is stalling, so I'm gonna close this one as well (not a lot of activity), but feel free to resubmit!

@cptroot
Copy link

cptroot commented Nov 26, 2018

I'm running into this issue right now where LLDB is trying to print a 591KiB byte slice everytime I step through my nom parser. Is there any way that I can revive this really old PR? Looking for guidance on what caused this to go unmerged in the past.

@tromey
Copy link
Contributor

tromey commented Nov 26, 2018

I think the gdb parts are probably not needed any more, at least not with a new-enough gdb (so that the struct printer is not used). You can control the number of printed elements in gdb with set print elements, and AFAICT all the other gdb pretty-printers read values lazily.

For lldb, I think the formatters need some updates to provide synthetic children. (This came up in a different bug as well; I'll file a bug for this specifically.) Then maybe the target.max-children-count setting will apply correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants