In D104488#2830992, @jingham wrote:

Are the Symbol ID's for unnamed symbols the same each time you read in a symbol file? While the unnamed_symbol symbol names are not significant, it would be good if you were crashing in __lldb_unnamed_symbol111 on one lldb run, you would also crash in the same unnamed symbol when you crashed again.

Just a few nits!

Fixed a case where the accessor was being used twice in the same function and fixed a typo in a comment.

In D104395#2828065, @OmarEmaraDev wrote:

@clayborg I tried implementing scrolling mechanisms as suggested. My first trial essentially defined a "visible area" rectangle which gets updated with every time the selection changes, then when it comes to drawing, each field checks if it is completely contained in the visible area and draws itself with an offset that we get from from the visible area origin. This worked, but fields that spans multiple columns can completely disappear leaving mostly no fields on the window, so it worked in most cases, but not all. My second trial was about drawing to an ncurses pad that is large enough to fit all contents, then the window is refreshed from that pad. This used manual ncurses calls because we don't support pads at the moment, so I scratched that for now. I think support for pads would be good for those kind of applications in the future. I plan to work on a proposal that would include support for pads and lightweight subwindows, I will detail that later.

Looks good!

Yes, good to go!

Lets try the diamond character for the boolean stuff unless anyone has any objections. Maybe handle a few more keys for the boolean field as suggested in the comments. This will be good to go after these changes!

Just one issue regarding the stop ID for post mortem. Once this is resolved, we will be good to go!

Looks good!

a few nits!

Looks very nice.

Accepting since no one else had comments. Sorry for the delay.

I mostly commented on ThreadTrace.h and we should resolve the questions in there before reviewing the rest of this patch.

Looks good!

Many minor things, but looking good! I look forward to seeing this get checked in

Once teemperor's issues are resolved this looks good to me!

One quick either documentation update for the error string, or switch to ConstString... Probably best to the the error string docs as if we use fancy C++ union stuff it might not compile for everyone on all C++ compilers..

So it would be nice to try and not encode errors into the TraceInstruction class and deal with any errors at decode time.

If no one else comments in the next 2 days, I will accept this patch. So speak up soon if you have an opinion.

Fix the test variable names and this is good to go

LGTM

In D102829#2781201, @simon.giesecke wrote:

@clayborg I am not sure what it means you now accepted the revision? I am still not sure what to do about the tests.

I am ok with all of the DWARF unwind changes. Someone else should give the ok for the MC stuff.

Do we want any safeguard on StoredDeclsList::prependDeclNoReplace(...)? Right now it can still cause duplicate NamedDecl pointers. I fix that tested helps prevent this:

The only last nit is we are passing the report progress callback around all over the place. I think it would be nicer to just make a static function that we can call and remove the passing of the instance variable for the report callback all over to the event classes.

I would be ok with this. Can you track down the original author and add them to the review of this to be sure they aren't using it anymore, and if they need tracing that they have everything they need with your new functionality?

Contents looks good, just need a test to ensure that warnings are quieted. There should be a test for each kind of warning, so it should be easy to run that same command with the --quiet option and verify that the warning does _not_ appear.

! In D102219#2755080, @simon.giesecke wrote:

Looks good to me. Thanks for the changes.

In D102634#2766582, @dblaikie wrote:

What's your use case such that this performance concern has come up for you?

I would go ahead and request commit access, but at least there is the Phabricator link in the commit that shows that you authored the diff. This is the main reason you want to get commit access, so you everyone can easily see who did the patches.

You should probably get Phabricator working: https://llvm.org/docs/Phabricator.html

No objections from me. Anyone else?

Was this all fixed by the auto linter?

I had run statistics on a few hundred DWARF files way back when and came up with the original 14-20 byte number, but this was a long long time ago (at least over 10 years). With newer DWARF versions, and with optimizations this can easily change. So it would be great to know what the minimum value should be set to by default. I agree that adding a statistic would be nice so we can track this. I will test some recent DWARF files out and see what my numbers show and report back in this patch.

In D102486#2759417, @simon.giesecke wrote:

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue here. I was wondering if we could avoid locking the mutex here, but using thread-local tables until finalisation does not seem to be feasible since any thread might insert the same string IIUC and we want to have unique IDs at this point. A lock-free map might be an alternative here, though?

In D102486#2760645, @grandinj wrote:

In D102486#2759417, @simon.giesecke wrote:

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue

Could perhaps start by moving the CachedHashStringRef construction outside the lock?

In D102219#2755080, @simon.giesecke wrote:

FWIW, and unrelated to this patch: processing other, more complex gcc-built binaries, e.g. clang, produces a lot of the other warnings (with and without the patch). Not sure if that's concerning.

In D102219#2753742, @simon.giesecke wrote:

In D102219#2752199, @clayborg wrote:

There are unit tests that could be added if you would like to test a bad case, but existing unit tests should still pass with this.

I had a look at the test cases in llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp. The TestDWARF* test cases seem to provide a template for such a test, but unfortunately I am not familiar with the details of DWARF to come up with a representative test case. With some advice, I could do that in another patch?

In D102219#2753743, @simon.giesecke wrote:

Also note this is my first contribution to LLVM and I don't have commit access, could you land the patch on my behalf?

In D102224#2753356, @simon.giesecke wrote:

In D102224#2752153, @clayborg wrote:

In D102224#2750014, @simon.giesecke wrote:

I know this isn't covered by tests right now. I am not sure what the policy is for the command line utilities. Currently, llvm-gsymutil doesn't have any tests IIUC. If tests should be added, please direct me to a good example, and I will be happy to add them.

yes tests should be added and there are indeed llvm-gsymutil tests. See this directory:

llvm/test/tools/llvm-gsymutil

Ah, sorry for missing that. It seems so obvious now that I don't know how I missed it.

In D102224#2750529, @simon.giesecke wrote:

Hm, clang-tidy complains about two issues in code I copied from llvm-symbolizer. Should these be addressed?

if it is new code, then yes.

One thing to note is that there is library code that tools can directly link to, just like llvm-gsymutil links against the "DebugInfoGSYM" library. Then your lookups are very simple and you would call a function just like doLookup() that you added. Is there a reason you are wanting to run a command line tool instead of linking against the code?

In a first step, we just wanted to switch from one command line tool (addr2line) to another. For some use cases, linking against the library would be an option, and probably indeed one we will take in a future step. In other use cases, this might be less feasible, notably pprof, which is written in Go, which also calls other command line utilities such as nm, addr2line or llvm-symbolizer.

Also, I am not a fan of text scraping from the output of a tool unless it is purely for humans to read. If you are going to use this output from another tools, I would vote to add a new option: "--json" so that the output would be formatted using structured data like JSON. Something like:

$ llvm-gsymutil --json --addresses-from-stdin
0x1000 /tmp/a.gsym
0x2000 /tmp/b.gsym

And this would return output in a specific JSON format, something like:

[
  { "lookupAddress": 4096, "gsym": "/tmp/a.gsym", ... },
  { "lookupAddress": 8192, "gsym": "/tmp/b.gsym", ... }
]

The "..." above would be a JSON version of the llvm::gsym::LookupResult object, or an error message.

I agree that having a JSON output would be better for further processing. I just wanted to keep changes minimal for now, and stick with the output format the tool has. Just after putting up this review, I noticed that https://reviews.llvm.org/D96883 added a --json option to llvm-symbolizer.

We should probably use the same format here.

That makes me wonder whether the direction this takes is actually the right one. Another option might be to add support for GSYM to llvm-symbolizer. Do you think this would be a better direction?

There are unit tests that could be added if you would like to test a bad case, but existing unit tests should still pass with this.

On optimization idea is that is one input file is specified, we could specify only addresses in the STDIN? Something like:

So looks like we just need tests:

• check error when user specifies --addresses-from-stdin and also an address
• check error when user specifies --addresses-from-stdin and also an input file
• check for successful lookups on multiple address + file tuples

And if would be good to specify the input format for the --addresses-from-stdin option in the option description.

Sounds like, after I reread the description, that we have other tools in the llvm ecosystem that use this <addr> <file> format... Sorry for the noise. I will add inline comments to clarify any needed changes.

In D102224#2750014, @simon.giesecke wrote:

I know this isn't covered by tests right now. I am not sure what the policy is for the command line utilities. Currently, llvm-gsymutil doesn't have any tests IIUC. If tests should be added, please direct me to a good example, and I will be happy to add them.

In D102085#2749112, @jingham wrote:

Was that what you had in mind, Greg?

Thanks for combining the functions and fine not to use Optional is we have a good value to indicate no timeout.

In D102085#2745450, @jingham wrote:

I don't see the change as "adding a bunch of timeouts", since what it mostly did was remove a bunch of send_async = false's. For all the clients that don't care about a timeout, I removed the send_async parameter, and for those that did I replaced it with a timeout. There were a few cases at the GDBRemoteClientBase layer where routines were calling the SendPacket routines with send_async = true w/o requesting that info from ProcessGDBRemote. To those I added a timeout parameter. That I consider a plus since it made explicit which packet request functions were interrupting and which were not.

Given the packet requests that called with send_async = false made up ~95% of all uses, it seemed like marking the few uses that did want to interrupt by the fact that they explicitly receive a timeout parameter was cleaner than switching everybody over to a Timeout, and then having 95% of the clients still have to make up an empty Timeout.

Instead, the rule is now "client functions that are going to interrupt the target get passed a timeout, and otherwise you don't have to worry about it". That removed a bunch of boiler-plate and seems clear to me.

We could go further, as you suggest, and add an m_interrupt_timeout to GDBRemoteClientBase (*). That would avoid ever having to pass a timeout into the SendPacket client functions in GDBRemoteClientBase. We already have ValueChanged callbacks for Properties. We'd have to add a virtual "InterruptTimeoutChanged" to Process to do the right thing for ProcessGDBRemote. None of that would be hard to do.

In fact, I actually thought about doing that, but I didn't because I like the fact that from ProcessGDBRemote, you are able to tell which methods in GDBRemoteClientBase interrupt a running process, and which don't by whether they take a timeout or not. If the timeout were in the GDBRemoteClientBase class you wouldn't see that. But that does seems a thing worth knowing. We could preserve that knowledge by adding some naming convention to distinguish between interrupting & non-interrupting methods. But I don't see what that really gains us.

Yes, lgtm!

LGTM! Just had one nit code edit suggestion.

Just fix the one issue where we use the FileSpec operator== and this is good to go!

If other architectures are emitting this language for assembly files, then this LGTM.

A bit of history on laying out classes: layout used to be a problem before we were able to give all of the field offsets directly to clang to assist in laying out. The main issues were:

• #pragma pack directives are not in DWARF so we must use the DW_AT_data_member_location
• Some DWARF optimizations cause class definitions to be omitted and we can have a class A that inherits from class B but we have no real definition for class B, just a forward declaration. In this case we will create a class A that inherits from a class B that has nothing in it, but the field offsets will ensure we show all other instance variables of class A correctly. Furthermore, we have code that can detect when we have such a case and it can grab the right definition for class B when it is imported into another AST, such as when evaluating an expression. This will only happen if class B is in another shared library from class A, and if we do have debug info for class B.
• any other keywords or attributes such as [[no_unique_address]] that can affect layout that don't appear in DWARF.

This will be tricky to do right and I really don't know how much extra performance we will get out of this for all of the possible issues we will can introduce by multi-threading things. That being said, I am happy to help see if this will actually improve performance.

I would be fine with DWARFDie getting bigger to 24B. These objects are used temporarily and not used as part of the storage of all of a DWARFUnit's DIEs. DWARFUnit objects store an array of DWARFDebugInfoEntry objects and those are what we care about not getting bigger.

Much of this patch requires all places that took a DWARFDie object now also must take an extra parameter. Is there really no way to have DWARFDie be able to discover the main unit via its contained DWARFUnit? DWARFDie objects are transient, so they can contain more information that just what they currently contain:

The title should be updated on this diff as well. Set the "json" right now