Page MenuHomePhabricator

labath (Pavel Labath)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 4 2013, 6:02 AM (294 w, 1 d)

Recent Activity

Today

labath added a comment to D57037: BreakpadRecords: Address post-commit feedback.

After some internal discussion, it seems that the situation with the all-zero UUIDs is as follows:

  • breakpad symbol files do not attach a special meaning to a zero UUID - if a file does not have a build-id, the dump_syms tool will use a hash of the first page of the text section (or something equally silly)
  • minidump files may treat the missing build-id by replacing it with zeroes depending on the tool used to produce the minidump: breakpad doesn't do that (it does the same hash as above), crashpad does.
Wed, Jan 23, 1:23 PM

Yesterday

labath added inline comments to D57037: BreakpadRecords: Address post-commit feedback.
Tue, Jan 22, 9:05 AM
labath added inline comments to D57037: BreakpadRecords: Address post-commit feedback.
Tue, Jan 22, 8:50 AM

Mon, Jan 21

labath updated the diff for D57037: BreakpadRecords: Address post-commit feedback.

rebase

Mon, Jan 21, 9:12 PM
labath committed rLLDB351781: breakpad: Add FUNC records to the symtab.
breakpad: Add FUNC records to the symtab
Mon, Jan 21, 8:56 PM
labath committed rL351781: breakpad: Add FUNC records to the symtab.
breakpad: Add FUNC records to the symtab
Mon, Jan 21, 8:56 PM
labath closed D56590: breakpad: Add FUNC records to the symtab.
Mon, Jan 21, 8:56 PM
labath added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Mon, Jan 21, 8:21 PM
labath added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Mon, Jan 21, 7:26 PM
labath added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Mon, Jan 21, 5:13 PM
labath resigned from D26295: Change UnwindAssemblyInstEmulation to remove a register location instead of marking it as IsSame() .
Mon, Jan 21, 4:49 PM
labath added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Mon, Jan 21, 4:46 PM
labath created D57037: BreakpadRecords: Address post-commit feedback.
Mon, Jan 21, 2:06 PM
labath added a comment to D56844: Breakpad: Extract parsing code into a separate file.

Thanks for the review. I'll create another review with the changes stemming from this.

Mon, Jan 21, 12:36 PM
labath added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Mon, Jan 21, 10:40 AM
labath committed rL351756: Fix compilation error with gcc 4.8.
Fix compilation error with gcc 4.8
Mon, Jan 21, 10:21 AM
labath accepted D57011: Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication.

This looks much better. Thanks.

Mon, Jan 21, 10:01 AM
labath added inline comments to D57011: Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication.
Mon, Jan 21, 7:06 AM
labath added a reviewer for D57011: Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication: sgraenitz.
Mon, Jan 21, 6:36 AM

Fri, Jan 18

labath committed rL351548: [ADT] Add streaming operators for llvm::Optional.
[ADT] Add streaming operators for llvm::Optional
Fri, Jan 18, 4:56 AM
labath closed D56795: [ADT] Add streaming operators for llvm::Optional.
Fri, Jan 18, 4:56 AM
labath added inline comments to D56795: [ADT] Add streaming operators for llvm::Optional.
Fri, Jan 18, 2:52 AM
labath committed rLLDB351541: Breakpad: Extract parsing code into a separate file.
Breakpad: Extract parsing code into a separate file
Fri, Jan 18, 2:41 AM
labath committed rL351541: Breakpad: Extract parsing code into a separate file.
Breakpad: Extract parsing code into a separate file
Fri, Jan 18, 2:41 AM
labath closed D56844: Breakpad: Extract parsing code into a separate file.
Fri, Jan 18, 2:40 AM

Thu, Jan 17

labath accepted D56618: [SymbolFile] Remove the SymbolContext parameter from FindTypes.

This makes sense to me.

Thu, Jan 17, 8:25 AM
labath added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
In D56230#1361650, @Hui wrote:

+#if defined(_WIN32)
+TEST(ArgsTest, GetFlattenWindowsCommandString) {
+ Args args;
+ args.AppendArgument("D:\\launcher.exe");
+ args.AppendArgument("--log=abc def");
+
+ std::string stdstr;
+ ASSERT_TRUE(args.GetFlattenWindowsCommandString(stdstr));
+ EXPECT_EQ(stdstr, "\"D:\\launcher.exe\" \"--log=abc def\"");
+}
+#endif

Thu, Jan 17, 8:13 AM
labath added inline comments to D55718: [ARC] Basic support in gdb-remote process plugin.
Thu, Jan 17, 8:07 AM · Restricted Project
labath added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.

I've always disliked this argument and hoped that someday someone would remove it entirely. My recollection (which may be wrong) is that the only actual use of it is so that if someone types a command, and we later need to print the command back, we will print it with the same quote char. It almost seems like we could just delete the argument and use a standardized quote char when flattening a command string.

Thu, Jan 17, 7:53 AM
labath added inline comments to D56795: [ADT] Add streaming operators for llvm::Optional.
Thu, Jan 17, 7:47 AM
labath committed rL351447: Recommit "Teach the default symbol vendor to respect module..
Recommit "Teach the default symbol vendor to respect module.
Thu, Jan 17, 7:12 AM
labath committed rLLDB351447: Recommit "Teach the default symbol vendor to respect module..
Recommit "Teach the default symbol vendor to respect module.
Thu, Jan 17, 7:12 AM
labath committed rLLDB351435: Recommit "Add a verbose mode to "image dump line-table" and use it to write a ..
Recommit "Add a verbose mode to "image dump line-table" and use it to write a .
Thu, Jan 17, 5:15 AM
labath committed rL351435: Recommit "Add a verbose mode to "image dump line-table" and use it to write a ..
Recommit "Add a verbose mode to "image dump line-table" and use it to write a .
Thu, Jan 17, 5:14 AM
labath planned changes to D56595: SymbolFileBreakpad: Add line table support.

I think I understand what you mean. I'll try to refactor this to create a compile unit for each function.

Thu, Jan 17, 5:08 AM
labath added inline comments to D56590: breakpad: Add FUNC records to the symtab.
Thu, Jan 17, 4:42 AM
labath updated the diff for D56590: breakpad: Add FUNC records to the symtab.

Thanks for the review. I've refactored the code to separate (and centralize) the
breakpad parsing from the part which does presents the information to lldb.

Thu, Jan 17, 4:39 AM
labath created D56844: Breakpad: Extract parsing code into a separate file.
Thu, Jan 17, 4:06 AM
labath added inline comments to D54617: [Reproducers] Add file provider.
Thu, Jan 17, 12:20 AM · Restricted Project
labath added a comment to D56814: [Reproducers] Refactor reproducer info.

This looks much better. LGTM, just make sure to do something with the lower_bound search, as I don't think that's right.

Thu, Jan 17, 12:14 AM

Wed, Jan 16

labath accepted D56798: Change TypeSystem::GetBitSize() to return an optional result..

The new "error" message definitely makes more sense. The rest of the patch seems fine too.

Wed, Jan 16, 12:27 PM
labath added inline comments to D54617: [Reproducers] Add file provider.
Wed, Jan 16, 12:22 PM · Restricted Project
labath added inline comments to D56795: [ADT] Add streaming operators for llvm::Optional.
Wed, Jan 16, 9:51 AM
labath created D56795: [ADT] Add streaming operators for llvm::Optional.
Wed, Jan 16, 9:46 AM
labath committed rL351359: Fix dir-separator-no-comp-dir-relative-name.s test added in r351328.
Fix dir-separator-no-comp-dir-relative-name.s test added in r351328
Wed, Jan 16, 9:43 AM
labath committed rLLDB351359: Fix dir-separator-no-comp-dir-relative-name.s test added in r351328.
Fix dir-separator-no-comp-dir-relative-name.s test added in r351328
Wed, Jan 16, 9:43 AM
labath committed rLLDB351353: Revert "Teach the default symbol vendor to respect module.GetSymbolFileFileSpec….
Revert "Teach the default symbol vendor to respect module.GetSymbolFileFileSpec…
Wed, Jan 16, 8:13 AM
labath committed rL351353: Revert "Teach the default symbol vendor to respect module.GetSymbolFileFileSpec….
Revert "Teach the default symbol vendor to respect module.GetSymbolFileFileSpec…
Wed, Jan 16, 8:13 AM
labath committed rL351330: Teach the default symbol vendor to respect module.GetSymbolFileFileSpec().
Teach the default symbol vendor to respect module.GetSymbolFileFileSpec()
Wed, Jan 16, 4:47 AM
labath committed rL351331: TestClangASTContext: Rewrite an if-else chain into a switch.
TestClangASTContext: Rewrite an if-else chain into a switch
Wed, Jan 16, 4:47 AM
labath committed rLLDB351331: TestClangASTContext: Rewrite an if-else chain into a switch.
TestClangASTContext: Rewrite an if-else chain into a switch
Wed, Jan 16, 4:46 AM
labath committed rLLDB351330: Teach the default symbol vendor to respect module.GetSymbolFileFileSpec().
Teach the default symbol vendor to respect module.GetSymbolFileFileSpec()
Wed, Jan 16, 4:46 AM
labath closed D56589: Teach the default symbol vendor to respect module.GetSymbolFileFileSpec().
Wed, Jan 16, 4:46 AM
labath committed rL351328: DWARF: Add some support for non-native directory separators.
DWARF: Add some support for non-native directory separators
Wed, Jan 16, 4:35 AM
labath committed rLLDB351328: DWARF: Add some support for non-native directory separators.
DWARF: Add some support for non-native directory separators
Wed, Jan 16, 4:34 AM
labath closed D56543: DWARF: Add some support for non-native directory separators.
Wed, Jan 16, 4:34 AM
labath updated the diff for D56543: DWARF: Add some support for non-native directory separators.
  • add a test for the no-DW_AT_comp_dir + relative-DW_AT_name case
Wed, Jan 16, 4:33 AM
labath committed rL351327: Revert "Simplify Value::GetValueByteSize()".
Revert "Simplify Value::GetValueByteSize()"
Wed, Jan 16, 4:23 AM
labath committed rLLDB351327: Revert "Simplify Value::GetValueByteSize()".
Revert "Simplify Value::GetValueByteSize()"
Wed, Jan 16, 4:23 AM
labath added a comment to D56721: Move llvm-objdump demangle function into demangler library.

But this does not protect us from anything in release builds.

Wed, Jan 16, 3:46 AM
labath committed rCTE351319: Fix build breakage from llvm r351317.
Fix build breakage from llvm r351317
Wed, Jan 16, 2:30 AM
labath committed rL351319: Fix build breakage from llvm r351317.
Fix build breakage from llvm r351317
Wed, Jan 16, 2:30 AM
labath committed rL351317: [Support] Remove error return value from one overload of fs::make_absolute.
[Support] Remove error return value from one overload of fs::make_absolute
Wed, Jan 16, 2:00 AM
labath committed rC351317: [Support] Remove error return value from one overload of fs::make_absolute.
[Support] Remove error return value from one overload of fs::make_absolute
Wed, Jan 16, 2:00 AM
labath closed D56599: [Support] Remove error return value from one overload of fs::make_absolute.
Wed, Jan 16, 2:00 AM
labath added a comment to D54617: [Reproducers] Add file provider.

Overall, I think the patch is pretty good. I made a bunch of inline suggestions/questions/comments, but they're all fairly minor. From a high-level view the two comments I have are:

  • I am slightly concerned about the non-temporality of this approach. Lldb does a fair amount of filesystem write operations, and this may be hard to capture in a static filesystem image. It seems you already had to work around some of these issues when you skip copying deleted files. I think that's a bridge we can cross when we reach it, but I'm just saying I see some potential trouble ahead...
  • I think it would be good nice to have a more granular test for this functionality. The existing test is sort of a sledgehammer (and apparently only runs on darwin). It is not obvious from it for instance, whether it actually tests any of the special symlink-handling code you have. (I guess it does, because you needed to write that code, but it's not clear whether that will still be true one year from now, or on someone else's machine). It sounds like it shouldn't be too hard to test the finer details of this implementation via unit tests, either by going through the FileSystem, or by going straight to the FileCollector class. What do you think?
Wed, Jan 16, 1:52 AM · Restricted Project

Tue, Jan 15

labath added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
In D56230#1358269, @Hui wrote:
  • drop the if (m_entries[i].quote) branch. You don't need it here, and I don't believe it will be correct anyway, because all it will do is cause llvm::sys::flattenWindowsCommandLine to add one more quote level around that and escape the quotes added by yourself

The quote char might be specified through Args::AppendArgument at user's will.

Tue, Jan 15, 11:48 AM
labath added inline comments to D56543: DWARF: Add some support for non-native directory separators.
Tue, Jan 15, 11:42 AM
labath added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
In D56230#1358102, @Hui wrote:

What do you think of the following codes to be added in Args?

bool Args::GetFlattenQuotedCommandString(std::string &command) const {
  std::vector<llvm::StringRef> args_ref;
  std::vector<std::string> owner;

  for (size_t i = 0; i < m_entries.size(); ++i) {
    if (m_entries[i].quote) {
      std::string arg;
      arg += m_entries[i].quote;
      arg += m_entries[i].ref;
      arg += m_entries[i].quote;
      owner.push_back(arg);
      args_ref.push_back(owner.back());
    } else {
      args_ref.push_back(m_entries[i].ref);
    }
  }

  command = llvm::sys::flattenWindowsCommandLine(args_ref);

  return !m_entries.empty();
}
Tue, Jan 15, 11:01 AM
labath accepted D56688: Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC).
Tue, Jan 15, 9:59 AM
labath added a comment to D56688: Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC).

Overall, I am pretty sure we will end up with some kind of a special Optional class sooner or later. However, it's not clear to me whether this is the place to start. What kind of space this is saving? I would expect that size matters when you store something as a data member in a class, because you can have zillion of instances of that class on the heap. However, all the uses of this I see are local (stack) variables, where the size does not matter that much, and so you might as well have used an Optional. Have I missed something?

Tue, Jan 15, 4:47 AM

Mon, Jan 14

labath added a comment to D55827: Update current working directory to avoid test crashes.

I think this problem ought to be fixed now (since D56545).

Mon, Jan 14, 4:57 AM · Restricted Project
labath added a comment to D56531: [CMake] Replace use of llvm-config with LLVM and Clang CMake packages.

it should be possible to build lldb against an already-installed llvm

When I wrote this I thought that it is possible to build against an installed LLVM, but I don't think that's currently possible. For example [...]

IIUC all LLVM subprojects are made for *testing* against the build-tree. LLDB tests depend on LLVMTestingSupport and others that are not part of the installed package (see LLVM_EXPORTS_BUILDTREE_ONLY).
While the ability to *build* against an installed LLVM sounds like a good goal in principle, it's questionable what benefit it has without the ability to *test* the result. Thus, I wouldn't consider it a high prio for now.

Mon, Jan 14, 4:56 AM
labath accepted D56615: [SymbolFile] Remove the SymbolContext parameter from FindNamespace.
Mon, Jan 14, 4:37 AM
labath accepted D56614: [SymbolFile] Change ParseFunctionBlocks(SymbolContext&) to ParseBlocksRecursive(Function&).

looks good to me.

Mon, Jan 14, 4:32 AM
labath added inline comments to D56609: [CMake] Remove dead code and outdated comments.
Mon, Jan 14, 4:25 AM

Sat, Jan 12

labath added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.

For example, for a Args vector like lldb-server, gdb-remote, --log-channels=foo\\\ \\\""" ''', whatever, QuoteForCreateProcess would return
lldb-server gdb-remote "--log-channels=foo\\\ \\\\\\\"\"\" '''" whatever (there are other ways to quote this too). Passing this string to CreateProcess will result in the original vector being available to the main function of lldb-server, which is what this code (and all other code that works with the Args class) expects.

Sat, Jan 12, 2:23 AM
labath added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.

I almost feel like the Args class could be made smarter here. Because all of the proposed solutions will still not work correctly on Linux. For example, why is this Windows specific? Args::GetCommandString() is very dumb and just loops over each one appending to a string. So if your log file name contains spaces on Linux, you would still have a problem no?

Sat, Jan 12, 2:15 AM
labath added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
In D56230#1354829, @Hui wrote:

I think the key problem here is to make sure the argument will be treated as a single argument to the process launcher.

Can you elaborate on that? I still don't see what is the problem with the solution I proposed.

Sat, Jan 12, 1:45 AM

Fri, Jan 11

labath committed rLLDB350947: Attempt to fix PDB tests broken by r350924.
Attempt to fix PDB tests broken by r350924
Fri, Jan 11, 10:28 AM
labath committed rL350947: Attempt to fix PDB tests broken by r350924.
Attempt to fix PDB tests broken by r350924
Fri, Jan 11, 10:28 AM
labath created D56602: Move FileAction, ProcessInfo and ProcessLaunchInfo from Target to Host.
Fri, Jan 11, 8:08 AM
labath created D56599: [Support] Remove error return value from one overload of fs::make_absolute.
Fri, Jan 11, 7:45 AM
labath added inline comments to D56543: DWARF: Add some support for non-native directory separators.
Fri, Jan 11, 6:50 AM
labath updated the diff for D56543: DWARF: Add some support for non-native directory separators.
  • move the comp_dir resolution logic from SymbolFileDWARF to DWARFUnit
  • this required the addition on an accessor to the "comp_dir_symlinks" setting, which is used in the full comp_dir resolution
  • add FileSpec::MakeAbsolute() to simplify bits of code
  • determine the path style from the DW_AT_name field if DW_AT_comp_dir is missing (I recall that it is possible to get a CU without a comp_dir in some circumstances, but I wasn't able to get my compiler to do that).
Fri, Jan 11, 6:42 AM
labath added inline comments to D56595: SymbolFileBreakpad: Add line table support.
Fri, Jan 11, 5:13 AM
labath created D56595: SymbolFileBreakpad: Add line table support.
Fri, Jan 11, 5:08 AM
labath created D56590: breakpad: Add FUNC records to the symtab.
Fri, Jan 11, 4:35 AM
labath created D56589: Teach the default symbol vendor to respect module.GetSymbolFileFileSpec().
Fri, Jan 11, 4:23 AM
labath resigned from D56586: [PPC64] Update LocalEntry from assigned symbols.

I suggest having this reviewed by the PPC target owner in llvm.

Fri, Jan 11, 4:18 AM
labath committed rL350924: Introduce SymbolFileBreakpad and use it to fill symtab.
Introduce SymbolFileBreakpad and use it to fill symtab
Fri, Jan 11, 3:21 AM
labath committed rLLDB350924: Introduce SymbolFileBreakpad and use it to fill symtab.
Introduce SymbolFileBreakpad and use it to fill symtab
Fri, Jan 11, 3:21 AM
labath closed D56173: Introduce SymbolFileBreakpad and use it to fill symtab.
Fri, Jan 11, 3:21 AM
labath committed rLLDB350923: ELF: Fix base address computation code for files generated by yaml2obj.
ELF: Fix base address computation code for files generated by yaml2obj
Fri, Jan 11, 2:22 AM
labath committed rL350923: ELF: Fix base address computation code for files generated by yaml2obj.
ELF: Fix base address computation code for files generated by yaml2obj
Fri, Jan 11, 2:22 AM
labath accepted D56564: [SymbolFile] Make ParseCompileUnitXXX accept a CompileUnit&..

looks good to me.

Fri, Jan 11, 12:27 AM
labath added inline comments to D56531: [CMake] Replace use of llvm-config with LLVM and Clang CMake packages.
Fri, Jan 11, 12:20 AM

Thu, Jan 10

labath committed rLLDB350834: Fix compilation error on 32-bit architectures introduced in r350511.
Fix compilation error on 32-bit architectures introduced in r350511
Thu, Jan 10, 7:59 AM
labath committed rL350834: Fix compilation error on 32-bit architectures introduced in r350511.
Fix compilation error on 32-bit architectures introduced in r350511
Thu, Jan 10, 7:59 AM
labath added a comment to D55434: ObjectFileBreakpad: Implement sections.

Thanks for the heads-up. This should be fixed in r350834.

Thu, Jan 10, 7:58 AM