- User Since
- Sep 23 2014, 10:11 AM (312 w, 5 d)
Fri, Sep 18
Right now all users of this code end up creating little extra functions that must work around this. lldb-vscode has it's own methods, the new IntelPT will do its own thing.
Tue, Sep 15
Mon, Sep 14
Wed, Sep 9
Thanks for the extra work arounds! LGTM. Pavel?
Tue, Sep 8
This looks good to me. We will need to wait until the JSON changes from https://reviews.llvm.org/D87335 to make it in though in case there are any modifications to that patch.
FYI: we switched away from "localhost" a long time ago due to issues with people having a "localhost" entry in their /etc/hosts folder.
Fri, Sep 4
Wed, Sep 2
I am not sure we need any of the classes in Trace.h: TraceInstruction, TraceDecodedThread, or TraceThread.
I think we should remove the "Stream *s;" from the TraceDumpOptions struct since we have a "Stream &s" as an argument already. We can't use a reference in the TraceDumpOptions without jumping through a lot of hoops.
Mon, Aug 31
I think we should probably make a TraceDumpOptions class to contain all of the dumping options and use that in the Target.h, Trace.h and all plug-ins. We will no doubt expand the number of options in the future and add more. We current have "tid" and "verbose". See inlined comments for details!
LGTM now. Probably should get an ok from one more person.
Fri, Aug 28
A few simple changes from inlined comments. Other than that looks good. Should get another OK from others. Also not sure if the JSON stuff in llvm needs to be done separately?
Thu, Aug 27
So we are adding a custom new "debugInfoSize" field to the "module" JSON dictionary? What happens in the release VS code? Does this information not get displayed, but it will get displayed in our custom modified VS Code IDE?
Wed, Aug 26
Added a better algorithm for detecting if an address from the module list for linux processes with valid /proc/<pid>/maps file is executable. We now search consective addresses that match the path to the module for any that are marked as executable. Added tests to test if the mmap'ed file comes first or second, and also a test for "-z separate-code" when the first mapping for an executable isn't executable, but a subsequent mapping with a matching path is.
Tue, Aug 25
Looking better. The main thing we need to do it modify StructuredData classes a bit by moving a lot of the static functions we are using here into the appropriate classes. See inlined comments.
Mon, Aug 24
Aug 21 2020
Lots of little things regarding the encoding and format of the JSON.
Added a test case with a 1 byte .text section that will overflow into the 15 bytes of the .data section that follows. This will test the overflow case that was requested.
Aug 20 2020
Patch looks good. Just need to test 32 bit RISC as well with a "lldb/test/Shell/ObjectFile/ELF/riscv32-arch.yaml" to verify 32 bit is working.
Hopefully this should be good to go, let me know if anyone has any issues.
- Use a safer method to read the data we need for the .text section in case we need and extra 15 bytes.
- Rephrase confusing comment
Aug 19 2020
All of the code looks good now and we need to add a test. This means we will need to modify vscode.py in the test suite to be able to received the reverse request and just launch the process. We don't need to launch the program in a terminal, just spawn the process using subprocess.
Aug 18 2020
Aug 17 2020
Adrian: is there something I need to do to enable simulator tests? I added a test to TestSimulatorPlatform.py but if I run:
Added a "platform process list" test to each simulator test to test this functionality.
LGTM, but Kuba should ok this one since he does more ASAN and TSAN stuff.
Sorry for the delay! LGTM.
Aug 14 2020
So there is a lot of noise in this patch that is just reformatting on code that hasn't changed. It would be nice to get rid of any changes that are whitespace/indentation/formatting only. Also see inlined comments for needed changes.
Aug 12 2020
Fix inline comment issues from vsk.
Aug 11 2020
The idea for the JSON file is that it must have a "name" (or maybe "plug-in"?) to identify which trace plug-in and a "trace-file" at the very least:
Aug 10 2020
Unresolved issues in this patch:
- For "trace load", I get the plugin for the JSON file by matching it up with the "name" field in the JSON, but I don't store the "trace_sp" anywhere. We will need to store it with the target that we create, or for later commands add it to a target that is stopped when the trace data is loaded via the process interface (through lldb-server is the current thinking for this).
- "trace dump" does nothing for now, but this is what we can use to test that "trace load" worked and was able to create a target.
Aug 5 2020
LGTM. Raphael? You have any more comments?
Aug 4 2020
Made DWARFDebugLine::Prologue::getLastValidFileIndex() and DWARFDebugLine::getLastValidFileIndex() return an Optional<uint64_t> and modified the error message for when a file index is out of range to specify the valid range, or specify that there are no files in the prologue file table. Added a test to cover this case as well.
Sounds like it is ok for tests to be here, looks good.
Ok, if other tests are in there, I am fine with this move.
lldb/test/API is for testing the public LLDB API functions that are exported as lldb::SB*. Not sure this belongs here. There was another patch that was moving things around as well where I commented with the same issue.
lldb/test/API is usually for testing the lldb::SB* interfaces IIRC. So not sure this move makes sense?
A few nits inlined.
I am not a cmake expert. Might be a good idea to check the git log for people with more cmake expertise?
Aug 3 2020
Looks better. Can we test this?
Jul 31 2020
Anymore issues with this patch?
Jul 30 2020
So there is a fundamental change needed for this patch to work. I tried to fully detail this in the inline comments. Let me know if you have any questions.
Added else braces back.
LGTM after fixing clang format related changes seen in "Lint: Pre-merge checks"
Jul 29 2020
This seems very x86 centric. I forwarded a few examples of ARM and ARM64 disassembly to Yifan so we can improve some of the regexes.
Thanks again for working on this. I will review any subsequent patches as they come in if you have any more contributions. Sorry for the long delay again, I had 3 months that I was out due to the head injury, hopefully this won't happen again!
Marked things as done.
Fixed inline comments:
- switched yaml files over to use ELF
- remove unneeded brackets
- renamed some variables
Jul 28 2020
Jul 27 2020
I really like the idea of coming up with a low PC that is the first valid .text address. This will filter out many of the zeroed out dead stripped DWARF and will make a cheap way for us to check addresses when parsing debug info and line tables. Checking for -1 and -2 of course is great to do as well.
Jul 25 2020
Yikes, sorry for not responding for so long. In the fall I was out on medical leave due to a head injury. Please feel free to ping more often if I do this again. I commented in the other patches (one accepted, and questions in the other). Let me know your thoughts.