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.

rebase

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

This looks much better. Thanks.

This makes sense to me.

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

In D56230#1358356, @zturner wrote:

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.

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

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.

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

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

• add a test for the no-DW_AT_comp_dir + relative-DW_AT_name case

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

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?

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.

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();
}

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?

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

In D56531#1355794, @sgraenitz wrote:

In D56531#inline-500764, @labath wrote:

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

In D56531#inline-501059, @xiaobai wrote:

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.

looks good to me.

In D56230#1355247, @labath wrote:

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.

In D56230#1354851, @zturner wrote:

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?

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.

• 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).

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

looks good to me.

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