In D77444#1963354, @labath wrote:

In D77444#1962952, @clayborg wrote:

The biggest issue is maintaining the API. We can't add anything to SBCommandPluginInterface

If we're going to be adding a new inheritable class for this feature, I'd recommend adding about half a dozen or so dummy virtual methods to it so we reserve vtable space for future expansion.

I was thinking if auto repeat was enabled that the DoExecute would just get called again with nothing and then the plug-in could maintain any repeat commands without needing to add anything... Glad you chimed in!

The biggest issue is maintaining the API. We can't add anything to SBCommandPluginInterface

LGTM. Pavel?

Does anyone know why harbormaster is useless?

In D74169#1960444, @avl wrote:

Not so much reason to support type units ("-fdebug-types-section/type units/.debug_types section/multiple .debug_info sections" all essentially describe the same thing). Since type units add object size overhead to reduced linked size in the face of a DWARF-agnostic/ignorant linker. If you have a DWARF aware linker, you'd probably avoid using type units so you could have smaller object files too.

It seems to me type units are useful for the current scheme. They increase the size of type reference(8 bytes vs 4 bytes) and introduce type unit header, but they avoid duplication. All references inside the object file point to the single type description - thus, object files have only one type's description instead of multiple copies. The effect of de-duplication is higher than the introduced increase.

In D74169#1960551, @dblaikie wrote:

In D74169#1960503, @clayborg wrote:

Nice! Another thing I have been seeing a lot of is when functions are coalesced and many .o files have the same function that gets linked to the same location. We end up with many duplicate DWARF DIEs in the final executable since the linker will just link all of the DWARF for each function over and over (.debug_info and .debug_line content). I found this when making GSYM files from the DWARF and noticing that I had hundreds of duplicate entries that matched exactly. This would be a great follow up patch for --gc-debuginfo

Given this is built on top of the DWARFLinker used for llvm-dsymutil which, I believe, already strips out those duplicate descriptions - hopefully that's already included in the functionality under review? But good to check/ask - @avl does this patch already do the good things for duplicate (or dropped) function sections?

(eg: "inline void f1() { } void f2() { f1(); }" + "inline void f1() { } void f2(); int main() { f1(); f2(); }" compiled and linked at -O0 the DWARF would usually contain two descriptions of 'f1', but ideally with a DWARF aware linker it'd contain only one description of 'f1'. Similarly: "void f1() { } void f2() { }" + "void f2(); int main() { f2(); }" compiled and linked with -ffunction-sections -Wl,-gc-sections would normally contain a DWARF description of "f1" but no code for f1, and a DWARF aware linker would strip out the DWARF description of 'f1' here)

Nice! Another thing I have been seeing a lot of is when functions are coalesced and many .o files have the same function that gets linked to the same location. We end up with many duplicate DWARF DIEs in the final executable since the linker will just link all of the DWARF for each function over and over (.debug_info and .debug_line content). I found this when making GSYM files from the DWARF and noticing that I had hundreds of duplicate entries that matched exactly. This would be a great follow up patch for --gc-debuginfo

commit 5998aceda9fdca04db4d9ee390cec660896bf0bf (HEAD -> master, origin/master, origin/HEAD)
Author: Greg Clayton <gclayton@fb.com>
Date: Thu Apr 2 16:30:33 2020 -0700

Fix typos.

Fixed patch reviewer suggestions and added a test for "id".

Another idea for the SymbolServers: be able to specify a source repository (git, svn etc) and hash or revision ID. The symbol server can grab the source from the repo and cache is locally for display.

This will do what the user intends more of the time. Good catch

Just use "llvm::None" instead of "{}" and this is good to go.

The main issue is that the symbol vendors currently are ELF, macOS and WASM. Right now we have one SymbolVendor for a triple, but I can see a SymbolVendor wanting to use multiple symbol servers to get information: one for the OS binaries (debuginfod or DebugSymbols.framework at Apple) and one for the current application with company specific symbol servers. At Apple, they can download any symbols for macOS, iOS, watchOS and tvOS OSes and applications. At Facebook we can download symbols for android, linux and iOS. Linux distros might have ways to download symbols for their OS stuff, which might work along side debuginfod? Also windows has the ability to download symbols.

I am worried if this test will be flaky on loaded machines. Not sure how we can ever guarantee we will see processor traces with our stuff in it if the machine is busy running many tests or even doing other things.

Looks good as long as with we have "llvm::Optional<llvm::StringRef> request_path = {}" in the arguments for a function, that "{}" will turn into llvm::None and not an empty StringRef? Unclear to me. As long as this is what is happening and as long is this coding convention is used elsewhere in llvm (using "{}" instead of "llvm::None") I am ok. Pavel?

After reviewing more, we should just re-use CreateBreakpoint and add a "llvm::Optional<StringRef> request_path" argument. Then all breakpoints use the same function and we avoid duplicated code. Inline comments should detail what should be done. Let me know if you have questions.

The description confused me a bit as I thought we were going to be doing some "CommandObjectSource::DumpLinesInSymbolContexts()" stuff somewhere. But this path is really just "return the same source path from the setBreakpoints" request in the response and I am all for that. So a few nits and this will be good to go. See my inline comments.

In D75750#1949527, @labath wrote:

In D75750#1948273, @clayborg wrote:

Currently we have a solution for macOS to locate symbol files in the "lldb/source/Symbol/LocateSymbolFile.cpp" file in the Symbols::LocateExecutableSymbolFile(...) function:

static FileSpec Symbols::LocateExecutableSymbolFile(const ModuleSpec &module_spec, const FileSpecList &default_search_paths);

This will locate any files that are already on the system and return the symbol file. When you don't have a symbol file, we can call:

static bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec &module_spec, bool force_lookup = true);

This might ping a build server and download the symbols.

As for source file remappings, as Jim stated, on mac, each dSYM has path remappings already inside of it that are applied on the Module (not a target wide setting) itself and no modifications need to be done to the SourceManager.

So my question is: can be use debuginfod to find the symbol file for a given build ID via Symbols::LocateExecutableSymbolFile(...), and when/if a symbol file is fetched from debuginfod, apply all path remappings to the module itself that we hand out? Then no changes would be needed in the SourceManager, we would just ask for a symbol file and get one back with all the remappings that are needed.

I've been thinking about that a lot too. The thing that's not clear to me is, does DownloadObjectAndSymbolFile download source files too? If so, how?

In D75750#1949678, @fche2 wrote:

In D75750#1949527, @labath wrote:

I am expecting that this feature will hook in very near to DownloadObjectAndSymbolFile for downloading the debug info, but it's not clear to me how would the source files fit in. Currently, debuginfod only provides an api to retrieve a single source file, so this code would have to parse all of the debug info, pry out the source files, and download them one by one -- a complicated and slow process.

Yeah, as debuginfod does not support a batch type of source download, maybe this particular lldb site is not an ideal fit..

Much cleaner!

Currently we have a solution for macOS to locate symbol files in the "lldb/source/Symbol/LocateSymbolFile.cpp" file in the Symbols::LocateExecutableSymbolFile(...) function:

LGTM as long as all test are passing!

Looks fine to me, but the original author should probably approve this

So I know the mach-o symbol table parsing code is a mess already, but it seems like this patch can be simpler if we make a std::set<lldb:addr_t> at the top of ObjectFileMachO::ParseSymtab() and every time we add a symbol that has a valid address value, add the file addr to this set. This will avoid the need to do any sorting. This std::set can be used to not add LC_FUNCTION_START entries that already have a symbol at the address, and it can be used to only add TrieEntry values that have symbols. Thoughts?

I would either add the option to SBLaunchInfo so it can be specified, or execute the command. If the target is created, it is setting a target specific setting. If had to pick I would add the API to SBLaunchInfo. Whenever I see something that can't be done through the API, I like to add that API if it is warranted. In our case the value in the SBLaunchInfo should probably be stored as a lldb_private::LazyBool which can have the following values:

Yes, this works now that we are creating the target in this function.

In D76111#1930783, @labath wrote:

The new interface is ok for me, but there are two things I want to mention:

• the iteration via Get{Name,Value}AtIndex is going to be slow because the underlying map (like most maps) does not have random access iterators. This is the problem I was trying to solve with the GetEntries-like API, but I agree that has its issues too.

lgtm.

Just watch the build bots carefully when you check this in!

Just some header doc cleanup before we submit. Thanks for sticking with these changes!

Jim is the one that really needs to mark this as accepted as the thread plans are one of his many areas of expertise.

Yep, copy and paste error!

We need to determine if the objects we return are copies (from SBPlatform and SBTarget), or if they are objects that will modify the environment for the platform and target respectively. If we are returning copies, we need setters on both the platform and target. If they are references, then they will just update the current environment. I think I would rather have the ability to modify them using the returned object, but I am not set on this if everyone else thinks otherwise.

Does this have something to do with a multiplexor that funnels traffic through one tunnel? I am curious as to why the ReadJSON() doesn't exit after the other side closes down?

I too ran into the AsmPrinter being a bit overkill for writing simple data. GSYM has a simple file writer class that just uses llvm::raw_pwrite_stream:

Everything looks good, just a question in inlined comment about having a thread plan hold onto a pointer to a thread. Seems dangerous

In D74636#1916356, @labath wrote:

There's a target.inherit-env setting in lldb (which I believe also works correctly for remote launches). Could you use that instead of reimplementing the feature in vscode?

lgtm once all formatting issues are taken care of. Pavel?

Change looks good, just needs a test. Should be easy to take a simple binary that has a .debug_aranges, and run obj2yaml on it, and tweak the segment size as needed?

As long as the keeping thread plans around for threads that aren't there is only enabled when OS plug-in is around, I am fine with this direction. One questions: do we currently only allow stepping on real threads or OS threads that are backed by real threads right now? And if an OS thread is backed by a real thread, do we associate the step plan with the OS thread ID or the real thread ID (the core)? If we use the OS thread ID, another way to potentially fix this is to know in a thread plan that we are stepping an OS thread, and avoid fetching the complete OS list of threads for non public stops, but only fetch the thread list for any OS threads that have active thread plans and any OS threads for threads that are active on a core. Then we don't end up fetching all OS threads all the time for stepping, but it does allow us to maintain and track our OS thread correctly. This would require changes to the OS plug-ins where getting the thread list needs to be able to grab only the OS threads that are on core, and we can manually fetch the OS thread info for any thread plans, and the plug-in would still need to be able to fetch the complete list on public stop events.

In D75390#1907713, @hctim wrote:

In D75390#1906642, @clayborg wrote:

I just need to be able to build my binary with -fsanitize=memory. Most clangs that we have installed do not support this. Is our solution to this really to create your own buildbot and run the test just like the buildbot would? Is there a toolchain I can download from somewhere? This really makes it hard to fix issues that come up.

Building the binary with -fsanitize=memory is basically it, yes. The tricky thing about MSan is that it requires an-MSan built libcxx/libcxxabi, which which isn't trivial to bootstrap for some configurations, hence I generally just point to the "how to repro exactly the bot". :)

Recommitted with

The full report you attached was enough to see what was going on. Is there a way we can enable always reporting the full output with FileCheck on the msan bots? That would help to see the full report, as we just got the first few lines in the buildbot error message.

I just need to be able to build my binary with -fsanitize=memory. Most clangs that we have installed do not support this. Is our solution to this really to create your own buildbot and run the test just like the buildbot would? Is there a toolchain I can download from somewhere? This really makes it hard to fix issues that come up.

Added architecture specific directories so when targets are not enabled, tests don't run.

Trying again with:

Fixed an issue causing buildbots to fail where strings from files in the file table could be added in an undefined order.

I figured out what was going on the build servers: function calls ordering in parameters lists is not defined which caused file table entries, which split string into directory and filename, to come out directory first, then basename, or basename then directory name. Should be an easy fix.

LGTM, check remote build failure to be sure!

Reverted with:

Is everyone ok with this naming?

These changes can easily be tested with:

./bin/llvm-lit -v ../llvm-project/llvm/test/tools/llvm-gsymutil

Remove an extra fix for triples that I will commit with a new patch.

Renamed back to llvm-gsymutil for everything. So this patch now is renaming from llvm-gsym to llvm-gsymutil for the tool directory.

Removed more dependencies from the "gsymutil" tool and removed uneeded function calls.

dblaikie and MaskRay: thanks for the code guideline and inline comments. I will fix this in an upcoming patch.

In D74883#1894309, @aprantl wrote:

Another thing that I realized too late: Is there a good reason to all it llvm-gsymutil instead of gsymutil? We only call llvm-dwarfdump llvm-dwarfdump to disambiguate it from other dwarfdump tools.