clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 10:11 AM (160 w, 1 d)

Recent Activity

Yesterday

clayborg accepted D38938: Logging: provide a way to safely disable logging in a forked process.

Much better.

Tue, Oct 17, 10:09 AM

Mon, Oct 16

clayborg added a comment to D38938: Logging: provide a way to safely disable logging in a forked process.

Never mind, re-reading your original comment made it clear.

Mon, Oct 16, 10:43 AM
clayborg added a comment to D38938: Logging: provide a way to safely disable logging in a forked process.

Who is holding the lock and then fork'ing? Seems like we should fix that? I thought all log calls should be "lock, log, unlock". Why is this causing problems?

Mon, Oct 16, 10:42 AM

Fri, Oct 13

clayborg added a comment to D38897: Add specific ppc64le hardware watchpoint handler.

Feel free to add me as a reviewer. Zach as well.

Fri, Oct 13, 1:04 PM
clayborg added a comment to D38897: Add specific ppc64le hardware watchpoint handler.

I agree with all of Zach's suggestions.

Fri, Oct 13, 1:04 PM

Tue, Oct 10

clayborg accepted D38394: Fix dumping of characters with non-standard sizes.

Looks good

Tue, Oct 10, 8:04 AM

Mon, Oct 9

clayborg accepted D38568: [lldb] Enable using out-of-tree dwps.

Looks good.

Mon, Oct 9, 1:18 PM
clayborg added a comment to D38568: [lldb] Enable using out-of-tree dwps.

(sorry for the delay, I was out on vacation for a week)

Mon, Oct 9, 8:36 AM
clayborg added a comment to D38323: Enable breakpoints and read/write GPRs for ppc64le.

Sorry for the delay, I was out on vacation for a week.

Mon, Oct 9, 8:32 AM
clayborg added inline comments to D38568: [lldb] Enable using out-of-tree dwps.
Mon, Oct 9, 8:29 AM
clayborg added a comment to D38394: Fix dumping of characters with non-standard sizes.

Looks good. Would be nice to add support for byte sizes of 3, 5 and 7 to the unchecked version as noted in inline comments, or remove the function if no one is using this function. Just a few quick fixes and this will be good to go.

Mon, Oct 9, 8:20 AM
clayborg accepted D38376: Update ABISysV_arm64::RegisterIsVolatile to accept registers prefixed with r.

Back from vacation, sorry for the delay.

Mon, Oct 9, 8:00 AM

Thu, Sep 28

clayborg accepted D38373: Add a few missing newlines in lldb-server messages.
Thu, Sep 28, 11:53 AM
clayborg added inline comments to D38323: Enable breakpoints and read/write GPRs for ppc64le.
Thu, Sep 28, 8:15 AM
clayborg added reviewers for D38323: Enable breakpoints and read/write GPRs for ppc64le: clayborg, labath.
Thu, Sep 28, 8:09 AM
clayborg added a comment to D38323: Enable breakpoints and read/write GPRs for ppc64le.

Looks fine. One main questions for new linux archs in particular: is linux using the lldb-server to debug these days even when debugging locally? If so, then this patch only needs to implement both a native register content and not the lldb_private::RegisterContext subclass.

Thu, Sep 28, 8:09 AM

Wed, Sep 27

clayborg added a comment to D38142: FreeBSD kernel debugging fixes.

Verify you have commit priveleges, and then check in the sources with the following text in the SVN commit message:

Differential Revision: https://reviews.llvm.org/D38142
Wed, Sep 27, 10:07 AM

Mon, Sep 25

clayborg accepted D38142: FreeBSD kernel debugging fixes.
Mon, Sep 25, 10:47 AM
clayborg added a comment to D38153: Inhibit global lookups for symbols in the IR dynamic checks.

Looks good.

Mon, Sep 25, 8:06 AM · Restricted Project

Fri, Sep 22

clayborg added a comment to D38142: FreeBSD kernel debugging fixes.

Thanks for the explanation of the private vs shared, we can leave it as you have it. Just rename to RelocateSection and this will be good to go.

Fri, Sep 22, 10:13 AM
clayborg requested changes to D38142: FreeBSD kernel debugging fixes.

Very nice section auto relocate code. Just a few things to clean up and this will be good to go.

Fri, Sep 22, 9:19 AM

Thu, Sep 21

clayborg added inline comments to D38142: FreeBSD kernel debugging fixes.
Thu, Sep 21, 2:58 PM
clayborg requested changes to D38153: Inhibit global lookups for symbols in the IR dynamic checks.
Thu, Sep 21, 2:03 PM · Restricted Project
clayborg requested changes to D38142: FreeBSD kernel debugging fixes.

So I found a lot more stuff. The inlined comments detail quite a few changes we should make. The ELF relocations can use a bit of cleanup. Let me know if anything is not clear.

Thu, Sep 21, 1:26 PM
clayborg requested changes to D38142: FreeBSD kernel debugging fixes.

Very close. See inlined comments.

Thu, Sep 21, 10:58 AM

Tue, Sep 19

clayborg added a comment to D37923: Implement interactive command interruption.

We should have a test. The test would need to use pexpect or something similar. If anyone has any pointer on how to make a test for this, please chime in. I was thinking just a pexpect based test.

Tue, Sep 19, 10:37 AM
clayborg accepted D37930: Use ThreadLauncher to launch TaskPool threads.
Tue, Sep 19, 8:34 AM

Mon, Sep 18

clayborg added a comment to D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump).

Looks fine.

Mon, Sep 18, 3:57 PM
clayborg added a comment to D37923: Implement interactive command interruption.

ok. Then back to the "can we use StringRef"? We might be able to check if the second.data() is NULL? It might be NULL if the string doesn't end with a newline. It it does, it might be empty but contain a pointer to the '\0' byte?

Mon, Sep 18, 3:51 PM
clayborg added a comment to D37923: Implement interactive command interruption.

I think Zach might be correct. If we already gathered the data, why not just output it all? Lets answer this first before returning to which implementation to use when parsing it up into lines and dumping it. Seems a shame to throw any data away.

Mon, Sep 18, 3:05 PM

Sep 18 2017

clayborg added a comment to D37934: Fix compatibility with OpenOCD debug stub..

I like the solution detailed above by tatyana-krasnukha

Sep 18 2017, 10:28 AM · Restricted Project
clayborg added a comment to D37934: Fix compatibility with OpenOCD debug stub..

If we get a response of "l" for the "qfThreadInfo", it might be worth trying to send a "H1" packet (select thread ID 1 for subsequent continues and steps) to see if the server knows about thread 1 before proceeding? This patch also broke other platforms that respond with "OK" to the "qfThreadInfo". So we really need to do some extra verification that "l" was the only things received in response to the "qfThreadInfo".

Sep 18 2017, 8:59 AM · Restricted Project
clayborg added a comment to D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump).

The default return value of WillResume should be no error. Sorry for not catching this. The core file Process subclasses will need to override this manually.

Sep 18 2017, 8:20 AM
clayborg accepted D33213: Use socketpair on all Unix platforms.

This should be good to go. Watch the buildbots for failures.

Sep 18 2017, 8:20 AM

Sep 15 2017

clayborg accepted D37934: Fix compatibility with OpenOCD debug stub..

As long as everyone agrees that no threads from qfThreadInfo means there is one thread whose thread ID is 1 then this is ok.

Sep 15 2017, 4:10 PM · Restricted Project
clayborg added a comment to D37934: Fix compatibility with OpenOCD debug stub..

The multiple feature fix is fine.

Sep 15 2017, 3:14 PM · Restricted Project
clayborg accepted D37930: Use ThreadLauncher to launch TaskPool threads.
Sep 15 2017, 2:10 PM
clayborg added inline comments to D37923: Implement interactive command interruption.
Sep 15 2017, 2:08 PM
clayborg added a comment to D37926: Fix the SIGINT handlers.

Looks fine to me.

Sep 15 2017, 1:50 PM

Sep 6 2017

clayborg added a comment to D37511: [dwarfdump] Verify line table prologue.

Why assert and not emit an error???

Sep 6 2017, 11:10 AM

Sep 5 2017

clayborg added a comment to D37492: Prevent vain lldb::user_id_t 0xffffffff lookups.

The top 32 bits is sometimes used to identify one of many DWARF files that are used to create a cohesive single view of the debug info such as when using DWARF in .o files on Mac, and other things like .dwo support and others. So the top 32 bits do mean something. I don't see any problem with this patch though. You will need to watch all of the buildbots closely after checking this in to make sure it doesn't affect other platforms that use these top 32 bits. Just because the test suite runs clean on your system, doesn't mean it will run smoothly on others. Let me know if you want to take this chance and be on the hook for fixes for other platforms in case things go south.

Sep 5 2017, 2:58 PM
clayborg accepted D37295: [lldb] Adjust UpdateExternalModuleListIfNeeded method for the case of *.dwo.

Just add a more descriptive comment that states this exact issue more clearly as I didn't understand the issue from the comment that is there now (the comment that starts on line 1644).

Sep 5 2017, 10:35 AM
clayborg added a comment to D37481: [DebugInfo] Verify that sibling and children fall in parent range..

I tend to use the DWARF generator to create a super minimal DWARF file, then obj2yaml that. It tends to be really short and ok for adding as a test. The DWARF generator only exists in the gtest for the DWARF, but you can add a new gtest, generate the DWARF, then remove the gtest.

Sep 5 2017, 10:09 AM
clayborg accepted D37441: Fix DW_FORM_strp parsing.

Yikes, I don't see how this was working at all, but it has been that way for a while? Good catch.

Sep 5 2017, 6:54 AM

Aug 31 2017

clayborg added a comment to D37295: [lldb] Adjust UpdateExternalModuleListIfNeeded method for the case of *.dwo.

Thanks for the info. Does this seem like it is a compiler bug? Seems like there should be a DW_AT_comp_dir if the DW_AT_GNU_dwo_name is relative? Is it written in the DWO spec that if the DW_AT_comp_dir is empty then it is referring to itself? I would rather not hack LLDB if we can avoid it. If this is a long released compiler generating this, then we do need a fix in LLDB since it will be running into these binaries from here on out. If this is a new compiler, lets fix the compiler. Let me know which case this is.

Aug 31 2017, 12:20 PM
clayborg accepted D37154: lldb-mi: -var-update can hang when traversing complex types with pointers.

Makes sense even though this can be quite expensive.

Aug 31 2017, 10:32 AM

Aug 30 2017

clayborg added inline comments to D37154: lldb-mi: -var-update can hang when traversing complex types with pointers.
Aug 30 2017, 2:59 PM
clayborg requested changes to D37154: lldb-mi: -var-update can hang when traversing complex types with pointers.
Aug 30 2017, 2:55 PM
clayborg added inline comments to D37154: lldb-mi: -var-update can hang when traversing complex types with pointers.
Aug 30 2017, 2:20 PM
clayborg added a comment to D37295: [lldb] Adjust UpdateExternalModuleListIfNeeded method for the case of *.dwo.

Can you explain the issue with an example?

Aug 30 2017, 12:57 PM
clayborg accepted D36804: Add initial support to PowerPC64 little endian (POWER8).
Aug 30 2017, 8:15 AM

Aug 28 2017

clayborg added a comment to D32366: Set "success" status correctly.

This is sure to trigger things in the test suite. We will need to ensure a few things:

  • test suite runs cleanly in debug mode after the lldbassert is added
  • without changes to CommandObjectRegister.cpp that the lldbassert is triggered, and if not, add a test
Aug 28 2017, 11:37 AM · Restricted Project
clayborg added a reviewer for D32366: Set "success" status correctly: labath.
Aug 28 2017, 11:08 AM · Restricted Project
clayborg added a reviewer for D32366: Set "success" status correctly: clayborg.
Aug 28 2017, 11:08 AM · Restricted Project
clayborg added a comment to D32366: Set "success" status correctly.

Yes: lldbassert would be fine for that since those get compiled out during release. Patch looks fine. If we already have a test that would trigger the new "lldbassert" you will add, then no need for a special test for this, else we need a test that triggers this.

Aug 28 2017, 11:07 AM · Restricted Project

Aug 23 2017

clayborg accepted rL311579: Process: fix FXSAVE on x86.
Aug 23 2017, 11:01 AM

Aug 22 2017

clayborg accepted D36804: Add initial support to PowerPC64 little endian (POWER8).
Aug 22 2017, 2:05 PM

Aug 21 2017

clayborg added a comment to D36620: Fix crash on parsing gdb remote packets.

We need a test for this to ensure we don't regress.

Aug 21 2017, 9:09 AM

Aug 16 2017

clayborg accepted D36046: Improve the posix core file triple detection.

Looks fine. Sorry for the delay.

Aug 16 2017, 10:37 AM

Aug 14 2017

clayborg accepted D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

Glad this worked.

Aug 14 2017, 8:16 AM

Aug 11 2017

clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

Might need to retitle again as well

Aug 11 2017, 9:32 AM
clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

So I looked at the instances of STT_NOTYPE in a few shared libraries on my computer and they do seem to have valid addresses in them.

Aug 11 2017, 9:32 AM

Aug 9 2017

clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

It would be a good idea to see what this $debug_ranges627 symbol actually is. It seems like a stray linker symbol that wasn't stripped? It would be a good idea to figure out what this symbol is so we can determine how to deal with it correctly.

Aug 9 2017, 11:04 AM
clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

I think a better solution is to not give the symbol's address a valid section in the first place. This would be done in ObjectFileELF.cpp when . It doesn't seem like the section is valid because $debug_ranges627 shouldn't have an address that matches something in .text. It just doesn't make sense.

Aug 9 2017, 11:01 AM
clayborg accepted D36445: Fix "process load/unload" on android.
Aug 9 2017, 10:49 AM

Aug 8 2017

clayborg added inline comments to D36445: Fix "process load/unload" on android.
Aug 8 2017, 8:01 AM

Aug 7 2017

clayborg accepted D33035: Tool for using Intel(R) Processor Trace hardware feature.

I was out on vacation, sorry for the delay. Looks good.

Aug 7 2017, 8:17 AM
clayborg accepted D36319: [DebugInfo][DWARF] Correct some usages of PRIx32 to PRIx64.

I was out on vacation, sorry for the delay. Looks good.

Aug 7 2017, 8:12 AM
clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

I was out on a week long vacation, sorry for the delay. The main questions is: should symbols with NOTYPE actually be included in any address lookups. I would vote for no as how can 1 address resolve to more than one section. That doesn't make sense to me. One address should resolve to one and only one section. Sounds like the NOTYPE symbols should just say they don't have a section. Thoughts?

Aug 7 2017, 8:12 AM
clayborg accepted D35760: Expose active and available platform lists via SBDebugger API.
Aug 7 2017, 8:07 AM · Restricted Project
clayborg accepted D36068: Add support for base address entries in the .debug_ranges section.

I was out on vacation, sorry for the delay. Looks good.

Aug 7 2017, 7:56 AM
clayborg accepted D33213: Use socketpair on all Unix platforms.

Remove the outdated comment and we are good to go.

Aug 7 2017, 7:54 AM

Jul 28 2017

clayborg added a comment to D11465: Fix "process load/unload" on android.

It would be nice to auto detect the names correctly?

Jul 28 2017, 12:00 PM

Jul 27 2017

clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

maybe lldb::SymbolType::eSymbolTypeCompiler?

Jul 27 2017, 12:03 PM
clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

So the issue is with the ObjectFileELF when it makes its symbol table. It is taking this symbols:

Jul 27 2017, 12:03 PM
clayborg accepted D33213: Use socketpair on all Unix platforms.

I'm ok as long as Pavel is ok. Please wait for the OK from him as well.

Jul 27 2017, 11:58 AM

Jul 25 2017

clayborg added a comment to D33213: Use socketpair on all Unix platforms.

So where did the other changes go where we use "--fd" for non Apple builds? Did those changes get lost? They will be needed.

Jul 25 2017, 9:08 AM
clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

Looking at an ELF file with DWARF, I see:

Jul 25 2017, 9:06 AM
clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

(in the above log output .text has a valid address, but .debug_aranges doesn't. We might need to cross correlate withe the program headers to tell if something gets loaded (PT_LOAD) for a given file address. What does the output of:

Jul 25 2017, 8:46 AM
clayborg added a comment to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

In your log we see:

Jul 25 2017, 8:46 AM
clayborg requested changes to D35784: [LLDB][MIPS] Set the Section's file address for ELF section to LLDB_INVALID_ADDRESS if SHF_ALLOC is not set.

Two issues we need to resolve:

  • I don't think AddressClass should have Absolute as a value. Absolute values in symbol tables are just numbers, not necessarily addresses.
  • This change won't work for ARM
Jul 25 2017, 8:40 AM

Jul 24 2017

clayborg accepted D35607: Extend 'target symbols add' to set symbol file for a given module.
Jul 24 2017, 2:52 PM · Restricted Project
clayborg added a comment to D35607: Extend 'target symbols add' to set symbol file for a given module.

Greg, are you with me checking this in?

Jul 24 2017, 2:51 PM · Restricted Project
clayborg added a comment to D35734: Don't allow LLDB to try and parse .debug_types.

This section have been already removed from Dwarf5 so I agree that we shouldn't spend too much time adding support for it.

Compilers wanting to use type units and DWARF 4 should be emitting them in the .debug_types section. DWARF 5 kept the concept but moved them back into the .debug_info section. Supporting type units in general seems like a good idea, and the only question is where they live and what the exact details of the header look like.

Jul 24 2017, 11:48 AM
clayborg added a comment to D35734: Don't allow LLDB to try and parse .debug_types.

The enum might need to be scoped outside the function or in a header file...

Jul 24 2017, 10:17 AM
clayborg added a comment to D35734: Don't allow LLDB to try and parse .debug_types.

I believe a crash looks like:

Jul 24 2017, 10:16 AM
clayborg requested changes to D33213: Use socketpair on all Unix platforms.

Please init fd

Jul 24 2017, 9:58 AM
clayborg added a comment to D35734: Don't allow LLDB to try and parse .debug_types.

We have been hitting this at Facebook for server apps that statically link the entire world (libc, libc++, libstdc++, all other needed shared libraries) as the debug info is too large unless -fdebug-types-section is used.

Jul 24 2017, 9:42 AM

Jul 21 2017

clayborg accepted D35740: Fix PR33875 by distinguishing between DWO and clang modules.

Looks like there already is a test case that was failing as Jim mentioned. Accepting based on that.

Jul 21 2017, 4:05 PM
clayborg created D35734: Don't allow LLDB to try and parse .debug_types.
Jul 21 2017, 1:31 PM
clayborg requested changes to D35607: Extend 'target symbols add' to set symbol file for a given module.

Looks fine, just add an error that checks that when the --file options is used, that only one argument (symbol file) is specified.

Jul 21 2017, 9:11 AM · Restricted Project
clayborg accepted D35525: Fix "help format" output.
Jul 21 2017, 9:03 AM
clayborg accepted D35622: Fix LLDB crash accessing DWZ (Fedora) debug info.

Very nice.

Jul 21 2017, 9:02 AM · Restricted Project

Jul 20 2017

clayborg added a comment to D35525: Fix "help format" output.

Add a test for this and this will be good to go.

Jul 20 2017, 1:30 PM
clayborg added a comment to D33035: Tool for using Intel(R) Processor Trace hardware feature.

I am not sure how sensitive typemaps are to the names of the arguments? Maybe we can just have you use different names and then the two typemaps won't collide? You can also write some manual code to do the remapping without typemaps.

Jul 20 2017, 9:55 AM
clayborg added a comment to D35607: Extend 'target symbols add' to set symbol file for a given module.

One other idea if we go with the UUID::Type solution: if we have UUIDs that are different, we could ask the ObjectFile (ObjectFileELF) to see if it thinks the files go together. Not sure if there is anything in the ELF file that would allow us to do that. In Mach-O we have all definitions for the .text and .data sections, just not the bytes. So it would be easy to compare the file address and file sizes of the sections that are in both to see if they match. Something like:

Jul 20 2017, 9:49 AM · Restricted Project
clayborg added a comment to D35607: Extend 'target symbols add' to set symbol file for a given module.

So we should just make sure this works:

(lldb) target symbols add --shlib a.out debug_binaries/a.out

The --uuid option is there to help you match up without having to specify the file path. If I am understanding correctly, "a.out" has no UUID or build ID, but "debug_binaries/a.out" does?

In this case, neither of the files has a build id/UUID. However, the tricky part here is that in this case we "invent" a UUID by doing a checksum of the whole file. This is done to support the .gnu_debuglink https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html method of finding symbols. It works like this:

  • if a file has a gnu_debuglink section, we set the UUID to the crc inside that section
  • if a file does not have a gnu_debuglink section, we *assume* it will be used as a gnu_debuglink target, and also set the UUID to the crc we compute from the whole file contents. This works fine for the gnu_debuglink case, as then the rest of the machinery will match the files based on their UUIDs. However, it does not work for the general case without the debug link, as then the files will end up with two different "UUID"s (because they are based on the file content), and everything will consider them as different.

    The solution that would seem best to me here would be to leave the UUID of files which don't have a build-id as empty, as the gnu_debuglink is not really a UUID -- it's main function is to contain a file path, and the crc just acts as a sanity check. This should make the logic for matching two uuid-less files saner, but it would require us to implement the gnu_debuglink method in a different way (which I think is fine, as the current way seems pretty hacky, and causes us to often compute crcs of large files needlessly). I have no idea how much of our existing code would blow up if we started having files without a uuid though.

    What do you think about that?
Jul 20 2017, 9:44 AM · Restricted Project

Jul 19 2017

clayborg added a comment to D35622: Fix LLDB crash accessing DWZ (Fedora) debug info.

I forgot the "return 0;" in the "if (!invalid_forms.empty()) {"

Jul 19 2017, 3:22 PM · Restricted Project
clayborg requested changes to D35622: Fix LLDB crash accessing DWZ (Fedora) debug info.

Good idea on passing back the unsupported form. Expand to all unsupported forms as suggested in inlined comments and this will be good to go.

Jul 19 2017, 3:18 PM · Restricted Project
clayborg requested changes to D35607: Extend 'target symbols add' to set symbol file for a given module.

So we should just make sure this works:

Jul 19 2017, 11:58 AM · Restricted Project