Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Paths

Table of Contentst

[DWARFYAML][debug_info] Rename some mapping keys. NFC.
AcceptedPublic
Actions

Authored by Higuoxing on Aug 5 2020, 3:33 AM.

Details

Reviewers

jhenderson
grimar
MaskRay
• espindola
alexander-shaposhnikov
rupprecht

Summary

This patch renames some mapping keys:

AbbrOffset -> DebugAbbrevOffset
AddrSize -> AddressSize
AbbrCode -> AbbrevCode

Diff Detail

Repository: rG LLVM Github Monorepo

Unit TestsFailed
View All

	Time	Test
	240 ms	linux > AddressSanitizer-x86_64-linux-dynamic.TestCases::alloca_big_alignment.cpp
	240 ms	linux > AddressSanitizer-x86_64-linux-dynamic.TestCases::alloca_detect_custom_size_.cpp
	240 ms	linux > AddressSanitizer-x86_64-linux-dynamic.TestCases::alloca_overflow_partial.cpp
	250 ms	linux > AddressSanitizer-x86_64-linux-dynamic.TestCases::alloca_overflow_right.cpp
	230 ms	linux > AddressSanitizer-x86_64-linux-dynamic.TestCases::alloca_underflow_left.cpp
		View Full Test Results (960 Failed)

Event Timeline

Higuoxing created this revision.Aug 5 2020, 3:33 AM

Herald added a reviewer: • espindola. · View Herald TranscriptAug 5 2020, 3:33 AM

Herald added a reviewer: alexander-shaposhnikov. · View Herald Transcript

Herald added a reviewer: rupprecht. · View Herald Transcript

Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

Herald added subscribers: llvm-commits, lldb-commits, cmtice and 2 others. · View Herald Transcript

Higuoxing requested review of this revision.Aug 5 2020, 3:33 AM

Herald added a subscriber: JDevlieghere. · View Herald TranscriptAug 5 2020, 3:33 AM

Harbormaster completed remote builds in B67070: Diff 283187.Aug 5 2020, 4:15 AM

What's the motivation for doing this?

In D85289#2195954, @jhenderson wrote:

What's the motivation for doing this?

We should make these mapping keys' name consistent with the spec.
The spec uses 'address_size', 'debug_abbrev_offset', 'abbreviation code' to refer these fields.

I see the point, but we don't do it for all fields in other contexts, and I have some mild concerns that DebugAbbrevOffset is unnecessarily verbose (I'd think AbbrevOffset would be sufficient. Perhaps it would be best to draw in one or two others? @JDevlieghere / @labath, any thoughts?

In D85289#2196062, @jhenderson wrote:

I see the point, but we don't do it for all fields in other contexts, and I have some mild concerns that DebugAbbrevOffset is unnecessarily verbose (I'd think AbbrevOffset would be sufficient. Perhaps it would be best to draw in one or two others? @JDevlieghere / @labath, any thoughts?

Yeah, I'm ok with these names. I would like to hear more people's ideas!

CC @jankratochvil who might also have thoughts on this topic :-)

Sorry about the delay, I was OOO.

I think that making these consistent with the DWARF spec is a good idea. It's true that this makes DebugAbbrevOffset a bit longish. I could also live with just AbbrevOffset, but I think the longer version is also fine for two reasons:

I expect most of this stuff to be created by copy-pasting
Ideally, I would want to avoid explicitly specifying this field in most cases, and instead rely on some sort of symbolic references between the sections (as discussed in the other reviews)

Okay, LGTM. I don't mind either way, and I suspect with the offset field becoming optional soon, it's unlikely to appear frequently, so the verbosity is a non-issue then.