- User Since
- May 9 2013, 11:10 AM (263 w, 2 d)
Fri, May 25
Upload patch to suppress checksums when we see a preprocessed file.
@trixirt do you mind if I commandeer this review? I think I have a patch.
LGTM with the indicated test tweak, but best if @filcab also takes a look.
It looks to me like the line-table emitted by the new path would be valid. But I am not comfortable reviewing things related to MCExprs and fragments.
Thu, May 24
I see that the per-file info does track the presence of # [line] N but it's not immediately obvious how to query "has any file seen one" which is really what I'd want to know. The file information has various levels of indirection...
Can we step back a second and better explain what the problem is? With current Clang the debug info for this function looks okay to me.
The store that is "missing" a debug location is homing the formal parameter to its local stack location; this is part of prolog setup, not "real" code.
Wed, May 23
(Replying here instead of inline as the thread is getting kind of long.)
After thinking about this a bit... The # directive will cause the filename to be identified as the source for the subsequent lines. That means it will show up as the original source file in the line table.
However, the compiler doesn't have the actual source file of the header (or the original source file, for that matter) and so cannot compute a correct checksum for it. That means, it should omit the checksum, for any file identified with a # directive.
I haven't looked at how CodeView handles this situation; for DWARF v5 I made inconsistent use of a checksum into an assertion offense.
Should we reconsider that? Or have the # directive cause Clang to omit checksums for *all* files, even the one it is actually reading?
@aprantl, @rnk, thoughts?
+ @rnk who did the initial checksum stuff for CodeView.
Tue, May 22
The code changes look pretty tidy. Let's sort out the -vv description and then we can call it good.
We require that the child's (instruction) address range is contained within the parent's address range; I think it's equally sensible to verify that the location-list ranges are contained within the address range.
I don't know that DWARF explicitly requires this contained-ness, but it's clearly implicit in some of the sections I looked at while trying to answer that question.
Addressed review comments prior to commit.
I take it this was accidental? If there are weaknesses in our documentation for how to use Phabricator, please let us know. I know it is not always straightforward (I had a number of issues when I tried to start using it).
Mon, May 21
Contrary to my usual practice, I did the emission and reading parts in the same patch, it was just easier to manage that way.
I could probably do the use-the-parsed-size stuff as a separate NFC if people want, but it didn't seem like that big a deal.
This wouldn't be another layout/ABI breakage, would it?
Fri, May 18
So on PS4, -fsanitize=vptr without an explicit -frtti will warn and disable the sanitizer, rather than implicitly enabling RTTI. As well as exceptions no longer implicitly enabling full RTTI.
In short, we never implicitly enable RTTI, which was the intention.
Tue, May 15
This is something that has needed doing, thanks! The code looks plausible to me, but I am not really an ELF expert so I am reluctant to formally approve it.
If there's a difference between the in-source name and the exposed-to-linker name, the in-source name should be provided to DW_AT_name and the exposed-to-linker name should be in DW_AT_linkage_name. Don't try to overload DW_AT_name with any kind of decoration imposed for linking purposes.
Mon, May 14
Fri, May 11
Use composition instead of inheritance.
Go for it. It sounds like an abstraction that hides the multiple-section nature of the beast will particularly help LLDB; on the LLVM side the dumper will want to remain a bit more cognizant of the underlying sections. But we can surely leverage ideas from each other, which will ultimately help with converging on the Grand Unified Parser.
I haven't given Pavel's comments as much thought as they deserve, but here are some of mine.
Thu, May 10
Does gold really preserve .debug_info and .debug_abbrev? Generally .debug_info is by far the largest DWARF section and so the one you most likely want to remove.
I can't coment on the xcode and python parts. The C++ all looks plausible.
I think the "sliding" trick will be trickier in the LLVM parser, as it has to handle .o files (which might have multiple instances of .debug_types in v4, and multiple instances of .debug_info in v5) but is conceptually nice. The sliding should be kept to DWARFDataExtractor and not pushed down into DataExtractor, IMO, because the problem is (I think) pretty specific to DWARF.
So, I've been attacking the type-units-in-.debug_info problem in LLVM, and what I've done (so far) in the LLVM parser is to factor out a DWARFUnitHeader class, and made DWARFUnit derive from that. This makes it feasible to parse a unit header before deciding what kind of unit we are looking at, which is necessary when .debug_info contains both type and compile units. I haven't posted that patch yet as it's an NFC prep for more work getting to the point of actually allowing mix-n-match units in .debug_info sections. But I'll post that patch today so people can at least look at it.
Tue, May 1
Mon, Apr 30
Fri, Apr 27
I am looking at making the LLVM parser handle type units across v4/v5, and the trick of pretending .debug_types is pasted onto the end of .debug_info seems like a pretty convenient fiction. It unifies DIE handling, and still allows the dumper to distinguish which bits are in which section, which is traditionally how the dumper UI thinks about DWARF. So as long as you and Jan are on the same page there, I'm happy to follow suit.
Apr 27 2018
Apr 26 2018
I'm not convinced that all of these errors/warnings are really parse-stoppers, but this patch is about the reporting mechanics and debating the merit of individual cases should happen separately.
@dblaikie or @JDevlieghere should do a final sign-off as they were the other main reviewers.
Apr 25 2018
Apr 24 2018
Apr 23 2018
Apr 20 2018
Apr 13 2018
So, it turns out DebugInfo/DWARF needs to start looking for multiple .debug_info sections, just like it looks for multiple .debug_types sections, because the type units are still in COMDATs. And I think I found the weird intermixing problem, that was a different bug.
Apr 12 2018
CUs don't come out right away... Huh. In my first attempt at emitting TUs to .debug_info, I just had MC reuse the .debug_info section when it was asked about .debug_types. And I was definitely seeing intermixed headers, and otherwise unparseable sections.
Apr 11 2018
The leading zeros are because an MD5 hash is defined to be a 128-bit value. MD5Result::digest() returns the full 32-character hex string. I didn't see any value in trying to suppress leading zeros, it's slow and more likely to be confusing than helpful.
The parsing should automatically zero-extend a short value, and as it's identical to .octa parsing (see test\MC\AsmParser\directive_values.s) I didn't bother with one for MD5.
Apr 9 2018
Apr 6 2018
Removing code with effectively no performance or functionality penalty is a beautiful thing. That part LGTM.
@jordan_rose still needs to sign off, I think.
Apr 5 2018
But OpenVMS will likely need other stuff, so if you would rather wait on a labels list until that happens, that's fine.
My understanding was that in the latest revision of the design we don't have anywhere to attach the DILabel to when it is optimized away. If we do care about DILabels that have been optimized away (I don't see why we would want to) we would have to add a list of labels to the DISubprogram similar to how we keep dead variables around. But I don't htink that that is worth it.
Apr 4 2018
Apr 1 2018
Is "trivial" the right flag? In DWARF v5 there is a "defaulted" attribute, which also distinguishes between in-class and out-of-class defaulting. I'd rather not have a bunch of different flags meaning only slightly different things if we can come up with a more coherent way to track the information we need.
Mar 30 2018
There's commentary in lib/MC/MCParser/AsmParser.cpp about the ungraceful degradation in SourceMgr's cache. Would this help or simplify what AsmParser is doing?
It's usual to do bitcode and text reading/writing for new metadata all in one patch; splitting it up actually makes it harder to review.
Needs a test. Probably should be combined with the Verifier patch as well.
This wants to have a test, which probably means it should be combined with one or more of the other patches that support creating/parsing the label metadata.
I suggest combining this with a patch that actually uses the API. We don't generally have commits that add APIs without uses or tests.
Mar 29 2018
Mar 28 2018
Mar 27 2018
Mar 26 2018
Mar 22 2018
I don't think requiring a new option is a great user interface. In existing use cases the location of the .dwo file matches the location of the output file. Why is this one different?
Mar 21 2018
LLD looks at .debug_line just to get improved diagnostics, yes? In which case the minimal .debug_info sections LGTM.