- User Since
- Jun 4 2013, 6:02 AM (294 w, 1 d)
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.
Mon, Jan 21
Thanks for the review. I'll create another review with the changes stemming from this.
This looks much better. Thanks.
Fri, Jan 18
Thu, Jan 17
This makes sense to me.
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.
Wed, Jan 16
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?
Tue, Jan 15
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?
Mon, Jan 14
I think this problem ought to be fixed now (since D56545).
looks good to me.
Sat, Jan 12
Can you elaborate on that? I still don't see what is the problem with the solution I proposed.
Fri, Jan 11
- 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.