Will the floating point emulation be perfect compared to actually JITing the code? If we are going to enable this feature I would like to see tests that actually JIT code do the same operations somehow to verify that things match up with the IR interpreted results produce. The main reason we didn't include floating point code in the IR interpreter before is it is harder to make things match up with exactly how things are going to be run if it were JIT'ed into the process. On x86_64 or x64 there are floating point settings that can be changed and affect how operations happen in the current thread. The main example I always think of is how older debuggers would create recursive descent parsers and evaluate floating point operations using native floating point types. I believe that our APFloat classes can do everything correctly, but I would like to see tests that verify that we get the same results as if we were running actual code in the process. For this you can add an extra function to the lldb/test/API/lang/c/fpeval/main.c source like:

typedef enum FloatOperation { add, subtract, multiply, ... };
double eval(double a, double b, FloatOperation op) {
  switch (op) {
  case add: return a+b;
  case subtract: return a-b;
  ...
  }
}

In D128557#3609545, @JDevlieghere wrote:

Thanks for pointing that out. I blindly copied the categories logic from Log::Disable which uses it when computing the flags. I've omitted it for now because I think it would be weird to set the circular buffer size to 5 and then have 0 messages printed for the process category because 5 log messages from the commands category pushed them out. One solution would be to keep a circular buffer per category. I'll think about it and if I come up with a good solution I'll put up another patch.

Seems to make sense to me, but I will let Jason Molenda give the final OK.

So we either need to track categories for each buffered log message in the circular buffer so that if the user specifies a category within a channel to dump with "log dump lldb <category>" we can filter out the messages we dump if they are not in the one or more categories that were supplied, or remove categories from the "log dump" command and only specify the channel. Easy to fix the code to work either way.

I don't have anything against this, though I still think the LogHandlers should use the Host layer by making the LogHandler objects into real LLDB plug-ins. But if everyone else feels otherwise, I won't object.

Maybe we should leave the option in the command for history purposes??? Taking it out will now make anyone's log enable lines fail when they didn't use to...

If we do add plug-ins, you could make an Apple specific plug-in named "darwin-os-log" or something that would only be compiled in for Apple builds, or we can do a generic "os-log" plug-in that uses the host layer.

So it seems like we are catering to our code organization here instead of doing the right thing and relying on the host layer. I would rather move the log code into the host layer or make log handlers actual plug-ins so that we can do the right thing here.

I think we should allow this class to always exist and not conditionally compile it in or out. Then we add a new Host.h method to emit a log message in "lldb/Host/Host.h" and allow each host OS to emit a message. Maybe there is a default implementation that emits the message to stderr, and then we override this for in HostInfoMacOSX.h? Then each OS can use their OS logging API of choice.

This is an interesting take that makes more sense than specifying a size in bytes and a "m_circular" since that kind of thing could cut the first message. Saving entire messages as they are logged might make more sense for our circular log messages as we would ensure we have full message lines, though it does come with a "copy each message" cost

Looks good!

In D128069#3599456, @dmlary wrote:

Sent this via email, but doesn't look like it showed up here:

I'm less concerned about the getter and the property and more about the underlying functionality. It doesn't look like it's being tested, and adding it to the SB API allows to change that.

Just to clarify, are you asking me to add a test for the underlying
functionality, Section.GetLog2Align()? Or is the goal to add the
test at the SBSection level to ensure we detect if someone ever
changes the underlying api?

LGTM, just a question if we want to restrict this test to linux only.

The delegate idea is fine too! No need to overload the base class with extra stuff if not needed. I would just like there to be no predispositions on the kinds of logs that can be used when buffering is enabled.

For this idea to work we would need to change the Emit() function to be DoEmit(...), then add logic to LogHandler:

void LogHandler::Emit(StringRef message) {
  if (m_circular) {
    // Fill buffer in circular fashion
    return;
  }
  if (m_buffer_size > 0) {
    if (m_buffer.size() + message.size() > m_buffer_size) {
      // If we exceed the buffer size, flush.
      DoEmit(m_buffer);
      DoEmit(message);
      m_buffer.clear();
    } else {
      // Buffer size not exceeded yet.
      m_buffer += message.str();
    }
  } else {
    DoEmit(message); 
  }
}

The main questions is if we want --buffered for any log channel (file, callback, or circular). If we add a --circular flag, then we just let things accumulate in the LogHandler class.

I could also imagine having multiple options. For example

Is there a test that was failing with this? If not can we add one?

LGTM!

Something like:

(lldb) log enable --buffered 100000 lldb process state thread step

If we do this, then we need to be able to specify which buffers to dump somehow. In case someone did:

(lldb) log enable --buffered 100000 lldb ...
(lldb) log enable --buffered 100000 dwarf ...

I the user types "log buffer dump" then all streams would be dumped, maybe with a prefix like:

"lldb" log messages:
...
"dwarf" log messages:
...

Or we might be able to specify one or more channels to get just the ones we want:

(lldb) log buffered dump lldb
(lldb) log buffered dump dwarf
(lldb) log buffered dump lldb dwarf

I would vote to add new options to "log enable" and enable any such features on a per "log enable" invocation if possible? Otherwise each time you want to enable this you need to do two commands, and if someone already enables buffered mode, then you wouldn't be able to log anything else? Lets say Xcode always enables logging with something like:

(lldb) log buffered enable 1000000
(lldb) log enable lldb process state thread step

Then the user doesn't know this and they try to enable logging with:

(lldb) log enable lldb expr

Then they currently wouldn't get any logging to the screen? It would be buffered until we dump it with "log buffered dump"?

Removed accessors from AddressRange class to keep it immutable.

Sorry for the delay, looks good with the test

ping

I would like to get to one array solution of LogMessagePart entries and avoid having two arrays of strings that need to stay in sync (rawTextMessages and evalExpressions). Simpler to understand and no need to document the interdependence of rawTextMessages and evalExpressions.

In D126254#3549946, @serge-sans-paille wrote:

Sorry for the back and forth: I find the logic difficult to follow. What would you think of something along these lines (untested) https://godbolt.org/z/vh356fhaP

Jim Ingham should give any final OK since he is the code owner of the thread plans.

Looks good to me, but someone that owns the DWARF parser in LLVM should give the final ok.

Updated the code to not pass over the input twice. We now check for invalid '=' characters in the decoding loop. Also changed how we strip extra bytes from the end of the decoded input by looking at the back of the Input string since we already detect invalid '=' characters in the main string and would have returned an error already.

In D126464#3545142, @PRESIDENT810 wrote:

I have refactored my code so it should looks cleaner now, but I'm not sure how to add a test. It seems that adding a test for thin archive on macOS platforms can be not so straightforward. I see that in lldb/test/API/functionalities/archives/Makefile, the test suite is using libtool instead of ar to create archives on macOS platforms, and I don't know whether I can produce a thin archive with that. Also, ld on macOS platforms seems to be unable to identify thin archives (I'm using ld64.lld when testing my code locally).

I would really appreciate it if someone can provide some advice about how to implement such a test case here, thank you!

A few things to fix but nothing serious. Nice patch! We also need a test for this. I would add a test to lldb/test/API/functionalities/archives/TestBSDArchives.py

address ranges look good to me.

In D126259#3535688, @jingham wrote:

In D126259#3535004, @clayborg wrote:

In D126259#3534997, @jingham wrote:

Don't allow setting signal actions by signal number before you have a process.

I understand

We don't know what signal 20 is going to end up being till we have a process, so allowing this by number doesn't make sense.

I am saying that it would be a good idea to make sure an error is returned when you do try and set a signal by number before a process exists and make sure there is a test that covers this if it isn't already in the current patch.

I think we crossed paths. I added that in the last update of the diff. The test is TestHandleProcess.py:20, and the code to enforce it starts around 1671 of CommandObjectProcess.cpp.

Address feedback:

• Handle any bad '=' characters separately before the main loop.
• Remember how many bytes to strip after decoding all bytes.
• Use "Inv" constexpr instead of the magic 64 number to make things more readable
• Update tests for changes

Ok, if we just protect the Collection::iterator stuff in AddessRanges, this will be good to go.

In D126259#3534997, @jingham wrote:

Don't allow setting signal actions by signal number before you have a process.

In D126259#3534919, @jingham wrote:

In D126259#3534729, @clayborg wrote:

So the "process handle" command allows us to set signals by signal number as well. Does this patch support this? It seems like it wouldn't be too hard to do if we wanted to handle this. Lemme know what you think, other than that LGTM.

Ah, I forgot about specifying the signal by number. Before you have a process I don't think we should allow signals by number. The mapping signal number -> "signal name for any platform" is not 1-1 so we couldn't guarantee we were doing the right thing here. I'll put in a check for "specified by number with no process" and error out.

In D123469#3533876, @avl wrote:

@clayborg Greg, Could you check&accept this review directly, please?

After requested refactoring is done(https://reviews.llvm.org/D123469?id=423119#inline-1190512)
the AddressRanges class was changed. It combines the ranges like this:

[0x100,0x200) and [0x200, 0x300) -> [0x100, 0x300)

It can affect llvm-gsymutil. I think it should be OK to combine ranges like this. Please check that behavior.

So the "process handle" command allows us to set signals by signal number as well. Does this patch support this? It seems like it wouldn't be too hard to do if we wanted to handle this. Lemme know what you think, other than that LGTM.

Don't use -1 as the invalid table value, use 64 to avoid triggering narrowing conversion errors.

In D126059#3532473, @avl wrote:

In D126059#3532264, @clayborg wrote:

A 1 to 2 percent improvement is probably going to go away if the asserts are enabled when it has to do the check each time. If would still hesitate to add this kind of API as it will encourage others to use the APIs. I would be ok with it if the methods are protected or private and if we can add the DWARFLinker as a friend class to allow access to these performant but dangerous APIs only in certain situations that really require it. I really want to avoid most people from being able to use these APIs if possible to make sure we don't end up with bugs. We had many many bugs that stemmed from mismatching DWARFUnit + DWARFDebugIntoEntry before the DWARFDie class was created, mostly when doing cross CU references or in Fission cases where you have the skeleton CU vs the actual DWO CU + a DWARFDebugInfoEntry that comes from the actual DWO file. So there is a real danger to the safety and correctness of the code here.

if all of these bugs could be caught by assertions then the problem probably does not look too severe?
Running debug version of the application will show the problems.

Anyway, if we do not want these methods to be used by others - I am OK to make these methods protected and make DWARFLinker the only one using them.

Removed an extra string that was added to Base64Test::Base64 during testing.

One small nit in the code you can choose to fix or not. LGTM. Thanks for the improvements!

Since this "optimized" value isn't part of the actual JSON schema for the stack frames, and since we aren't using it in the IDE, best to remove this until it is either in the protocol definition or used by the IDE.

A 1 to 2 percent improvement is probably going to go away if the asserts are enabled when it has to do the check each time. If would still hesitate to add this kind of API as it will encourage others to use the APIs. I would be ok with it if the methods are protected or private and if we can add the DWARFLinker as a friend class to allow access to these performant but dangerous APIs only in certain situations that really require it. I really want to avoid most people from being able to use these APIs if possible to make sure we don't end up with bugs. We had many many bugs that stemmed from mismatching DWARFUnit + DWARFDebugIntoEntry before the DWARFDie class was created, mostly when doing cross CU references or in Fission cases where you have the skeleton CU vs the actual DWO CU + a DWARFDebugInfoEntry that comes from the actual DWO file. So there is a real danger to the safety and correctness of the code here.

I would actually request that no one can play with DWARFDebugInfoEntry items at all except for the DWARFDie class. The main issue is you can ask the wrong DWARFUnit class to do something with a DWARFDebugInfoEntry object that it doesn't own. We ran into this problem in LLDB and this is why we created the DWARFDie class in LLDB. If you encapsulate everything into the DWARFDie class and control all entry points, then this mistake can not be made since everyone needs to use the DWARFDie class. If you do want it add it, I would suggest adding an assert that verifies that the DWARFDebugInfoEntry belongs to the current DWARFUnit in each new function that takes a "DWARFDebugInfoEntry *", but I would still rather see everyone use DWARFDie if possible for safety reason and to avoid having an assert in each call.

A little background: we have people that end up debugging optimized code and when a variable's SBValue had an error, we wouldn't show anything in the variables window for the value.

Thanks for the changes. LGTM

Looking great, just a few more fixes. Thanks for making the changes.

It might be a good idea to add a test for this. There seems to be a test already in this file:

lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py

In fact looking at this test, it seems they specify the "SaveCoreStyle" via an option. As long as we are adding new APIs to SBProcess, we should probably add the "SaveCoreStyle style" as a parameter to the new SaveCore, this enum is already in the public header file (lldb-enumerations.h).

LGTM, but I don't own the LLVM JSON parser, so someone else should give the final ok!

Remove logging change from VSCode.cpp that was accidentally left in.

Much easier and cleaner. LGTM

probably, we would like to use both dwarflinkers with dsymutil and dwarfutil. Thus, we do not want to name tools in the option. How about these variants:

--dwarf-linker={classic,mt}  (mt is multy-thread)

--dwarf-linker={classic,with-joined-types}

--dwarf-linker={classic,experimental}

lgtm!

My main concern with creating one large unit for all types is the performance of the debugger when consuming this, but we don't need to fix this in the first iteration and can work on improving this later after we prove it is a problem. As long as the DWARF is in good shape and we are properly uniquing types in a much better way than before I am good.

Another solution would be to make m_thread_should_exit a std::atomic<bool> type instead of just a bool.

We can't change any public API calls since other tools might link against the existing API, but we can add new variants to the API. Inline fixes have been suggested.