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.

Thanks for the extra work arounds! LGTM. Pavel?

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.

In D87333#2262236, @rupprecht wrote:

In D87333#2262229, @clayborg wrote:

FYI: we switched away from "localhost" a long time ago due to issues with people having a "localhost" entry in their /etc/hosts folder.

I assume you mean people *not* having a "localhost" entry?

FYI: we switched away from "localhost" a long time ago due to issues with people having a "localhost" entry in their /etc/hosts folder.

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.

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.

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?

In D86670#2244092, @labath wrote:

I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.

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?

In D85705#2241249, @labath wrote:

If the format is going to be json, why not use llvm::json from llvm/Support/JSON.h? That's what we been migrating most of the existing stuff to already, so going using that for new code makes perfect sense.

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.

In D85705#2237397, @JDevlieghere wrote:

A large part of this patch is concerned with parsing which worries me from a maintenance perspective. Did you consider using Yaml I/O? While I'm not a particularly big fan of the format, the benefits of being able to (de)serialize any class by implementing the appropriate traits are quite significant. We already have traits implemented for a bunch of utility classes, such as ArchSpec and FileSpec which we could reuse for this. I know changing the format would be invasive, but I think it might be worth it in the long term.

After speaking with Walter a bit, key names in the JSON should be camel case. Many languages (Swift and JavaScript) can auto import JSON and create objects and use the key names as member variable names, and if they are camelCase, then no name conversion needs to happen.

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.

In D86375#2233054, @labath wrote:

If I correctly understand what this is doing, then I don't think it's a good idea. The base of an (elf) shared library does not have to be mapped executable. These are the mappings I get for a trivial hello world program (no mmapping of libraries or anything) on my linux machine:

00400000-00401000 r--p 00000000 fd:01 2838574                            /tmp/l/a.out
00401000-00402000 r-xp 00001000 fd:01 2838574                            /tmp/l/a.out
00402000-00403000 r--p 00002000 fd:01 2838574                            /tmp/l/a.out
00403000-00404000 r--p 00002000 fd:01 2838574                            /tmp/l/a.out
00404000-00405000 rw-p 00003000 fd:01 2838574                            /tmp/l/a.out
020fb000-0211c000 rw-p 00000000 00:00 0                                  [heap]
7fe4f5d87000-7fe4f5da9000 r--p 00000000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5da9000-7fe4f5ef3000 r-xp 00022000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5ef3000-7fe4f5f3d000 r--p 0016c000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5f3d000-7fe4f5f41000 r--p 001b5000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5f41000-7fe4f5f43000 rw-p 001b9000 fd:01 2738932                    /lib64/libc-2.31.so
...

Here, the correct base of a.out is 0x00400000 and the libc base is 0x7fe4f5d87000. But this patch would make them be detected as 0x00401000 and 0x7fe4f5da9000, respectively.

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.

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.

Fixed:

• 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

In D85988#2226911, @JDevlieghere wrote:

In D85988#2226889, @clayborg wrote:

In D85988#2226496, @aprantl wrote:

In D85988#2222616, @clayborg wrote:

Adrian: is there something I need to do to enable simulator tests? I added a test to TestSimulatorPlatform.py but if I run:

$ ../debug/bin/llvm-lit -sv lldb/test/API/macosx/simulator
llvm-lit: /Users/gclayton/Documents/src/lldb/mono/llvm-project/lldb/test/API/lit.cfg.py:147: warning: Could not set a default per-test timeout. Requires the Python psutil module but it could not be found. Try installing it via pip or via your operating system's package manager.

Testing Time: 19.61s
  Unsupported: 1

1 warning(s) in tests

It is unsupported? I am running this on a mac. Do I need to launch a simulator first? Extra arguments to the test suite?

No, the tests are supposed to run. Green dragon (which is running a consumer macOS + consumer Xcode) is running this test. I know because it sometimes fails because it couldn't launch the simulator :-(
The test was XFAILed a few weeks ago — is your LLVM up-to-date?

It was up to date. Can you run my invocation on your lldb?:

../debug/bin/llvm-lit -sv lldb/test/API/macosx/simulator

That works for me, but it looks like you're using the standalone/Xcode build, so maybe that's borked somehow :(

In D85988#2226496, @aprantl wrote:

In D85988#2222616, @clayborg wrote:

Adrian: is there something I need to do to enable simulator tests? I added a test to TestSimulatorPlatform.py but if I run:

$ ../debug/bin/llvm-lit -sv lldb/test/API/macosx/simulator
llvm-lit: /Users/gclayton/Documents/src/lldb/mono/llvm-project/lldb/test/API/lit.cfg.py:147: warning: Could not set a default per-test timeout. Requires the Python psutil module but it could not be found. Try installing it via pip or via your operating system's package manager.

Testing Time: 19.61s
  Unsupported: 1

1 warning(s) in tests

It is unsupported? I am running this on a mac. Do I need to launch a simulator first? Extra arguments to the test suite?

No, the tests are supposed to run. Green dragon (which is running a consumer macOS + consumer Xcode) is running this test. I know because it sometimes fails because it couldn't launch the simulator :-(
The test was XFAILed a few weeks ago — is your LLVM up-to-date?

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.

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.

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.

Fix inline comment issues from vsk.

In D85705#2211073, @vsk wrote:

This looks very cool, thanks @clayborg! I think using JSON to describe the trace data (what kind of trace is this, what's in it, etc.) sounds reasonable.

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).

Have you considered what might happen if there are multiple targets covered by a single trace? Strawman proposal: would it make sense to register the trace with a Debugger instance? This can be a list of traces if it makes sense to support debugging more than one trace at a time.

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:

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.

LGTM. Raphael? You have any more comments?

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?

Looks better. Can we test this?

Anymore issues with this patch?

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"

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

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.

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.