Page MenuHomePhabricator

probinson (Paul Robinson)
User

Projects

User Details

User Since
May 9 2013, 11:10 AM (384 w, 6 d)

Recent Activity

Yesterday

probinson added a comment to D68620: DebugInfo: Use base address selection entries for debug_loc.

TL;DR: It's all good.

Wed, Sep 23, 8:55 AM · Restricted Project

Thu, Sep 17

probinson added a comment to D68620: DebugInfo: Use base address selection entries for debug_loc.

(as an aside: do you/@probinson have much of a sense of how much binary size increase you'd trade for object size reductions in this space? If binary size is ulmtimately what you care most about, I'm guessing maybe debug_loc base address specifiers will never be a win for you & perhaps we should just group them under a flag, maybe refactor the debug_ranges base address specifier flag to cover both (the flag there was introduced due to a gold+gdb_index+32 bit binary bug, unfortunately, but lumping them together seems OK-ish to me))

I'm not sure, I'll defer to @probinson on that.

Thu, Sep 17, 8:19 AM · Restricted Project
probinson added inline comments to D87656: [llvm-dwarfdump] --show-sources option to show all sources.
Thu, Sep 17, 7:39 AM · Restricted Project

Mon, Sep 14

probinson updated subscribers of D68620: DebugInfo: Use base address selection entries for debug_loc.

@probinson - do you have any state here? Whether this is an ongoing issue, whether the improvements to avoid unnecessary debug_loc have reduced the overhead sufficiently, etc?

Mon, Sep 14, 8:09 AM · Restricted Project

Tue, Sep 1

probinson accepted D86965: Do not emit "-tune-cpu generic" for PS4 platform.

Couple of nits and LGTM.

Tue, Sep 1, 12:28 PM · Restricted Project

Aug 6 2020

probinson added a comment to D85239: [DOCS] Add more detail to stack protector documentation.

Whoa--that's the *help* text? Well, if that's the only documentation for options that there is, I guess it has to go there; but it seems pretty excessive for the (ideally concise) output of clang --help.

Aug 6 2020, 10:51 AM · Restricted Project

Aug 4 2020

probinson added a comment to D85239: [DOCS] Add more detail to stack protector documentation.

Looks okay (one grammar nit), probably worth waiting for someone else to chime in.

Aug 4 2020, 12:46 PM · Restricted Project

Jul 30 2020

probinson added a project to D84802: Add control over debug lines dropping: debug-info.
Jul 30 2020, 10:46 AM · debug-info, Restricted Project
probinson added a comment to D84802: Add control over debug lines dropping.

No test?

Jul 30 2020, 10:46 AM · debug-info, Restricted Project

Jul 28 2020

probinson accepted D68061: [docs] Document pattern of using CHECK-SAME to skip irrelevant lines.

@rupprecht if you are still motivated to land this after so long, the only thing I see missing is to update the example from CHECK-SAME: 1{{$}} to CHECK-SAME: {{ 1$}} and then it LGTM. If not, there seems to be enough interest that someone else can land it?

Jul 28 2020, 7:38 AM · Restricted Project

Jul 23 2020

probinson accepted D83462: [DWARF] Avoid entry_values production for SCE.

LGTM

Jul 23 2020, 8:19 AM · Restricted Project, debug-info

Jul 20 2020

probinson added a comment to D84094: [DebugInfo] Fix misleading using of DWARF forms with DIELabel. NFCI..

It looks like you have been tweaking a number of related SizeOf() methods. Is it practical to have just one? In a parent class, or even as a free function (as the size computation depends only on form/format, and is not related to the class at all).

Jul 20 2020, 8:56 AM · debug-info, Restricted Project
probinson added a comment to D84106: [llvm-readobj] Construct relocation-aware DWARFDataExtractor to decode .eh_frame addresses correctly.

I agree with @grimar as it looks like the change is from a DWARFDataExtractor without relocations to one with relocations.
Regarding header order, in cases like this it's usual to do an NFC commit to sort the headers that are there, and then the functional patch just adds the new one in the right place.

Jul 20 2020, 8:45 AM · Restricted Project

Jul 15 2020

probinson added a comment to D83834: Add test utility 'split-file'.

Just wanted to say I agree this can be better than the Inputs/ idiom, especially for comparatively short inputs. We have some downstream tests that use a bunch of echo commands to generate a second file, but an extract utility would be cleaner IMO.

Jul 15 2020, 6:38 AM · Restricted Project

Jul 14 2020

probinson added a comment to D83468: [Debuginfo] Fix for PR46653.

According to https://bugs.llvm.org/show_bug.cgi?id=46653#c3 there is no problem on x86-64, suggesting the fix is more likely to be in the AArch64 target instead of in generic code. It's not uncommon for passes to lose source locations, or fail to add them when they should, so that is more likely to be the cause of the AArch64 issue.

Jul 14 2020, 12:28 PM · debug-info, Restricted Project
probinson added inline comments to D83468: [Debuginfo] Fix for PR46653.
Jul 14 2020, 12:25 PM · debug-info, Restricted Project
probinson added a comment to D83725: [llvm-mc] Add --doc-id=<id> to support multiple documents in a file.

Wouldn't using .ifdef directives with --defsym on the command line work equally well? At least for .s files run through llvm-mc.
There are examples of this in the lit tests already, easy enough to find.

Jul 14 2020, 6:13 AM · Restricted Project

Jul 9 2020

probinson accepted D83091: [FileCheck] Improve -dump-input documentation.

LGTM.

Jul 9 2020, 2:09 PM · Restricted Project
probinson added a comment to D83462: [DWARF] Avoid entry_values production for SCE.

So the tuning here for SCE is also a "does not support" or something else?

Jul 9 2020, 2:01 PM · Restricted Project, debug-info
probinson added inline comments to D83468: [Debuginfo] Fix for PR46653.
Jul 9 2020, 1:32 PM · debug-info, Restricted Project
probinson added a comment to D83462: [DWARF] Avoid entry_values production for SCE.

Between this code and DwarfDebug, I'm having a hard time understanding the various controls over emitting entry-values.

  • There's an existing tuning check in DwarfDebug, now you're adding the complementary check in TargetOptions. Does this make the DwarfDebug check redundant?
  • There are 3 different flags to sort out whether to emit this stuff. SupportsDebugEntryValues is strictly based on the target, I get that. Then there's EnableDebugEntryValues in the TargetOptions, which seems to be tied to a tool-command-line option; and separately there's EmitDwarfDebugEntryValues in DwarfDebug, with its own command-line option. Can we simplify this at all?
Jul 9 2020, 7:33 AM · Restricted Project, debug-info
probinson accepted D83463: [DWARF][EntryValues] Emit GNU extensions in the case of DWARF 4 + SCE.

Thanks! This will keep the debugger folks off my case. :-)

Jul 9 2020, 7:17 AM · Restricted Project, debug-info

Jul 6 2020

probinson added a comment to D82975: [DebugInfo] Allow GNU macro extension to be emitted.

I think if it's about compatibility(analogous behavior with GCC), existing infra is Okay/Fine(Since same encodings are used). We just need to emit the .debug_macro section with version 4 and teach the llvm-dwarfdump to parse it correctly.

One difference though is that the GNU extension does not have anything like the strx entries that LLVM currently emits: https://github.com/gcc-mirror/gcc/blob/master/include/dwarf2.h#L425, so I assume we still need code to emit the strp entries when targeting DWARF 4?

Likely - but might want to check what GCC does - maybe it uses some kind of strx encoding that's not documented, etc.

Jul 6 2020, 2:48 PM · Restricted Project, debug-info
probinson added a comment to D82129: [DebugInfo] Drop location ranges for variables which exist entirely outside the variable's scope.

I think I didn't fully grasp that the blocks were being (tail-)merged, which makes the scope ambiguous, and all the rest. So I withdraw the objection on that basis. DWARF is fine with multiple variables pointing to the same location, but it's less forgiving about scopes IIRC, much like it can't describe multiple source attributions for an instructions. This all makes me sad, but that's how DWARF is at the moment.

Jul 6 2020, 2:06 PM · debug-info, Restricted Project

Jul 1 2020

probinson added a comment to D82828: [ELF] Don't resolve a relocation in .debug_line referencing an ICF folded symbol to the tombstone value.

I agree that being able to set a breakpoint on each different source of an ICF is useful, and that the COMDAT case typically has the same source and can use the tombstone.

Jul 1 2020, 9:10 AM · Restricted Project

Jun 30 2020

probinson added inline comments to D80242: [Clang] implement -fno-eliminate-unused-debug-types.
Jun 30 2020, 3:46 PM · Restricted Project

Jun 25 2020

probinson added a comment to D82129: [DebugInfo] Drop location ranges for variables which exist entirely outside the variable's scope.

Looking closer at COFF/register-variables.ll, it doesn't look like a bug but instead another victim of how we model debug info. Before running -branch-folder (Control Flow Optimizer) all the instructions in the else block belong to the else block scope. The branch folder merges the common tails from 'then' and 'else' into 'else', merging the debug-locations while it does so. @jmorse summarised the situation well offline: Every time we call getMergedLocation, we are creating the conditions where this occurs, and eliminating it during compilation would be a high burden.

Jun 25 2020, 1:05 PM · debug-info, Restricted Project

Jun 24 2020

probinson added a comment to D82129: [DebugInfo] Drop location ranges for variables which exist entirely outside the variable's scope.

This patch depends on the ranges for all scopes to be (reasonably) correct,

I'd say instead that 'variable locations depend on the ranges for all scopes to be (reasonably) correct'. And that this patch just acknowledges that relationship and clears away what we cannot use/see in a debugger.

Either way, the question remains: when we find cases where we need to "clear away" something, is that a bug, or is this merely a cleanup pass? In the case of the test I commented on, it's a bug, and I'd rather not be hiding bugs.

Jun 24 2020, 8:04 AM · debug-info, Restricted Project

Jun 23 2020

probinson added a comment to D82363: [DebugInfo] Add new instruction and expression operator for variadic debug values.

DBG_VAL_VAR stands for variadic, any alternative for the name? VAR may indicate to 'variable' I think...

DBG_VALUE_VARIADIC seems reasonable though also quite long, but it's better than being misleading.

Jun 23 2020, 9:37 AM · Restricted Project, debug-info

Jun 22 2020

probinson added a comment to D82129: [DebugInfo] Drop location ranges for variables which exist entirely outside the variable's scope.

This patch depends on the ranges for all scopes to be (reasonably) correct, but I think there's one modified test where that's not the case; a variable location is dropped because (AFAICT looking at the equivalent DWARF output) its containing scope isn't pointing to the right instructions.
If we take the stance that these cases are bugs, then this analysis is better off done in a verifier, so we can find and fix those cases, rather than papering over compiler bugs.

Jun 22 2020, 10:12 AM · debug-info, Restricted Project
probinson added a comment to D81784: [ELF] Resolve relocations in .debug_* referencing (discarded symbols or ICF folded section symbols) to tombstone values.

I've been watching, and have nothing to add to what James or Dave had to say.

Jun 22 2020, 10:12 AM · Restricted Project

Jun 18 2020

probinson added a comment to D81198: [docs] Specify rules for updating debug locations.

We recently ran into a case where it looks like the variable's location was correct but the containing scope was wrong. I guess we'll need a separate update for how to manage scopes.
Not an objection to this patch, just observing there are more cases to think about.

Jun 18 2020, 9:45 AM · Restricted Project
probinson accepted D82055: [DebugInfo] Explicitly permit addr_size = 0x02 when parsing DWARF data.

In a .test file, you don't actually need the comment markers. They're not harmful either, so up to you whether to leave them in.
Some people prefer that commentary and action lines (REQUIRES, CHECK, etc) have different comment markings, just for readability.

Jun 18 2020, 9:45 AM · Restricted Project

Jun 17 2020

probinson added a comment to D82017: [DebugInfo/DWARF] - Do not hang when CFI are truncated..

Convert to using cursor instead?

Jun 17 2020, 9:08 AM · Restricted Project
probinson accepted D81437: [CGP] Reset the debug location when promoting zext(s).

Definitely better, thanks!

Jun 17 2020, 9:08 AM · Restricted Project
probinson added a comment to D79507: [flang] Change DIE("unreachable") cases to use llvm_unreachable.

I fully support having a requirement that LLVM should support being shipped in Release with LLVM_ENABLE_ASSERTIONS configurations as a first class citizen, and would support an RFC on this on the LLVM mailing list.

Jun 17 2020, 9:08 AM · Restricted Project, Restricted Project
probinson accepted D81970: [Clang][Driver] Remove gold linker support for PS4 toolchain.

LGTM with one inline comment.

Jun 17 2020, 6:58 AM · Restricted Project

Jun 15 2020

probinson accepted D81469: [DebugInfo/DWARF] - Report .eh_frame sections of version != 1..

If the .eh_frame section contains contributions for multiple compilation units, the dumper should skip over the bad one and continue dumping at the next one. In which case, you'd need only one test, with multiple units in it (imitating a linked executable).

Sounds reasonable.

But Error DWARFDebugFrame::parse(DWARFDataExtractor Data) returns errors for a few cases already,
e.g. for the case when something is wrong with the augmentation string. Instead it could provide a way to
report a warning (perhaps a warning callback) and continue dumping.

Reimplementing the DWARFDebugFrame::parse() to support warnings should be done independently from this change I think?

Jun 15 2020, 1:14 PM · Restricted Project
probinson accepted D80381: Fix debug line info when line markers are present inside macros..

Without this patch, I'm seeing line 109 reported, which makes no sense at all. The test shows with the patch we get line 105, which in effect means that the line directive is processed as if it was not inside a macro definition; that makes some sense.
LGTM

Jun 15 2020, 12:37 PM · debug-info, Restricted Project
probinson accepted D81144: [MC] Generate .debug_line in the 64-bit DWARF format [2/7].

Ping. Are you OK with this patch and the whole series or there is something I should improve?

Jun 15 2020, 12:04 PM · debug-info, Restricted Project
probinson accepted D81476: [DWARF5] Enable .debug_line.dwo section emission if macro info is present.

LGTM.

Jun 15 2020, 12:04 PM · debug-info, Restricted Project

Jun 10 2020

probinson updated subscribers of D80242: [Clang] implement -fno-eliminate-unused-debug-types.

+debug-info tag

Jun 10 2020, 12:13 PM · Restricted Project
probinson added inline comments to D81476: [DWARF5] Enable .debug_line.dwo section emission if macro info is present.
Jun 10 2020, 11:05 AM · debug-info, Restricted Project

Jun 9 2020

probinson added a comment to D59553: [WIP][LLD][ELF][DebugInfo] Use predefined value to mark debug address ranges pointing to deleted code..

Ah @MaskRay posted the link to the DWARF issue ( http://www.dwarfstd.org/ShowIssue.php?issue=200609.1 ) in another thread.

Jun 9 2020, 3:31 PM · lld, Restricted Project
probinson added a comment to D59553: [WIP][LLD][ELF][DebugInfo] Use predefined value to mark debug address ranges pointing to deleted code..

I've filed a DWARF issue to reserve "-1" as the tombstone address value. I wrote it up such that consumers should ignore that address and any address derived from it (e.g., base+offset). I don't have the issue number for it yet, hopefully I'll remember to post it here when it's assigned.

Jun 9 2020, 10:25 AM · lld, Restricted Project
probinson added a comment to D81469: [DebugInfo/DWARF] - Report .eh_frame sections of version != 1..

If the .eh_frame section contains contributions for multiple compilation units, the dumper should skip over the bad one and continue dumping at the next one. In which case, you'd need only one test, with multiple units in it (imitating a linked executable).

Jun 9 2020, 8:12 AM · Restricted Project
probinson added inline comments to D81198: [docs] Specify rules for updating debug locations.
Jun 9 2020, 8:12 AM · Restricted Project
probinson added a comment to D81144: [MC] Generate .debug_line in the 64-bit DWARF format [2/7].

If an assembler source contains .file/.loc directives, and -dwarf64 is given, llvm-mc will still correctly produce a DWARF64 .debug_line section. That is because the only differences between DWARF32 and DWARF64 ones are unit_length and header_length fields, and references to .debug_line_str, and all these are covered by this patch.

Do you have any specific example which results in an incorrect output with these patches?

Jun 9 2020, 7:39 AM · debug-info, Restricted Project
probinson added a comment to D81437: [CGP] Reset the debug location when promoting zext(s).

That's a pretty long test case for a one-line patch; there's no existing zext testcase where you can just add !dbg metadata?

Jun 9 2020, 7:05 AM · Restricted Project

Jun 8 2020

probinson added reviewers for D81422: Change filecheck default to dump input on failure: jdenny, thopre.

I don't remember the exact reasoning but I believe it had something to do with bot logs? @jdenny or @thopre might remember.

Jun 8 2020, 1:18 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
probinson added a comment to D59553: [WIP][LLD][ELF][DebugInfo] Use predefined value to mark debug address ranges pointing to deleted code..
In D59553#1627199, @avl wrote:

@echristo @aprantl @probinson

Folks, I would like to ask a question related to this review and DWARF standard:

DWARF5 standard has the following statement related to non-continuous address ranges:

2.17.3 Non-Contiguous Address Ranges
...
A bounded range entry whose beginning and ending address offsets are equal
(including zero) indicates an empty range and may be ignored.

Would not it be useful to extend it to explicitly explain the following case
(for some next version of the standard)

"A bounded range entry whose beginning address offset greater than ending address offset
indicates an invalid range and may be ignored. "

?

This situation (LowPC > HighPC) already represents an invalid range.

llvm-dwarfdump --verify :
error: Invalid address range [0xfffffffffffffffe, 0x0000000000000004)

Thus it would be useful to indicate that explicitly and describe that default handling would be to skip.
So that broken ranges do not affect valid ranges.

For this patch, it would mean that practically all cases
would lead in this situation, and DWARF readers would adequately skip them :

LowPC -> points to deleted section -> set to UINT64_MAX-1; 
HighPC -> points to deleted section -> set to UINT64_MAX-1; 

ignored.
LowPC -> points to deleted section -> set to UINT64_MAX-1; 
HighPC -> points to correct section -> set to some value less than UINT64_MAX-1;

ignored.
LowPC -> points to deleted section -> set to UINT64_MAX-1; 
Length has some value -> HighPC = UINT64_MAX-1 + value -> overflow -> less than LowPC

ignored.

What do you think?

Jun 8 2020, 1:18 PM · lld, Restricted Project
probinson added inline comments to D81198: [docs] Specify rules for updating debug locations.
Jun 8 2020, 12:41 PM · Restricted Project
probinson added a comment to D81144: [MC] Generate .debug_line in the 64-bit DWARF format [2/7].

(By "hand-written assembler" I mean assembler source without .file/.loc directives. llvm-mc is perfectly capable of assembling a file with .file/.loc directives but that will take different paths not covered by this patch. It was not at all clear from the patch description that only the former case was intended.)

Jun 8 2020, 8:48 AM · debug-info, Restricted Project
probinson added a comment to D81144: [MC] Generate .debug_line in the 64-bit DWARF format [2/7].

I accept that generating DWARF64 format for hand-written assembler source has use-cases.

Jun 8 2020, 8:12 AM · debug-info, Restricted Project
probinson added a comment to D81389: dwarf::isCPlusPlus fails with user language.

FTR, we like to have full context diffs as described here: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

Jun 8 2020, 7:39 AM · Restricted Project

Jun 4 2020

probinson added a comment to D81145: [MC] Generate a compilation unit in the 64-bit DWARF format [3/7].

This patch is for generating DWARF-64 format for raw assembler source. Why is that an interesting case? I could see wanting to emit a .debug_info section for higher level languages in DWARF-64 format, but that is not what this patch does.

Might be best to keep the fundamental design direction/question here in one thread ( https://reviews.llvm.org/D81144 ) rather than in all of them. Patches are in a series, so if that one ends up going another direction I don't think there's a risk these later ones will be reviewed/approved/committed in spite of that fundamental question.

Jun 4 2020, 6:46 PM · debug-info, Restricted Project
probinson added a comment to D81146: [MC] Generate .debug_aranges in the 64-bit DWARF format [4/7].

So if we want DWARF-64 .debug_aranges for both asm source and higher-level language source....
This patch only does the asm source path, not the main path. I am not 100% clear where the other path is, but I see code to emit .debug_aranges in lib/DWARFLinker/DWARFStreamer.cpp, is that where the normal path is now?

Jun 4 2020, 6:46 PM · debug-info, Restricted Project
probinson added a comment to D80382: [MC] Implement generating 64-bit debug info..

Ah sorry hadn't noticed this one was abandoned. Never mind.

Jun 4 2020, 1:52 PM · debug-info, Restricted Project
probinson added inline comments to D81147: [MC] Generate .debug_rnglists in the 64-bit DWARF format [5/7].
Jun 4 2020, 1:51 PM · debug-info, Restricted Project
probinson added a comment to D81145: [MC] Generate a compilation unit in the 64-bit DWARF format [3/7].

This patch is for generating DWARF-64 format for raw assembler source. Why is that an interesting case? I could see wanting to emit a .debug_info section for higher level languages in DWARF-64 format, but that is not what this patch does.

Jun 4 2020, 1:51 PM · debug-info, Restricted Project
probinson added a comment to D81144: [MC] Generate .debug_line in the 64-bit DWARF format [2/7].

The test is verifying that llvm-mc can emit a DWARF-64 line table describing assembler source that does not have any .loc directives in it. That's not really the interesting case; the interesting case is assembler source that *does* have .loc directives in it, because that's the path that most compilations will actually use. Describing assembler source really doesn't need to support DWARF-64.

Jun 4 2020, 1:50 PM · debug-info, Restricted Project
probinson added a comment to D80382: [MC] Implement generating 64-bit debug info..

Some of these changes are specific to cases where we are emitting DWARF describing assembler source; I don't see any reason to support DWARF-64 for those cases. (In general, any functions with GenDwarf in the name are for this case.) Also in most cases the DWARF version for sections describing assembler source are hard-coded to DWARF version 2, which did not define the DWARF-64 format.

Jun 4 2020, 1:50 PM · debug-info, Restricted Project
probinson added a comment to D81146: [MC] Generate .debug_aranges in the 64-bit DWARF format [4/7].

This patch modifies EmitGenDwarfAranges(), which is used only when the assembler is generating DWARF to describe the assembler source.
I can't imagine anyone is really writing large enough assembler that they need to emit DWARF-64 format. What is the motivation here?

Jun 4 2020, 1:50 PM · debug-info, Restricted Project

Jun 2 2020

probinson added a comment to D80945: [DebugInfo] Fix a fatal error originating from split-macro support.

The correct fix is to emit the .debug_line.dwo section (which repeats the directory/file tables from the .debug_line section in the .o file) and have .debug_macro.dwo point to it. "If a DW_MACRO_start_file entry is present, the header contains a reference to the .debug_line section of the compilation." (p.169) Yes, it would be clearer to mention both normal and .dwo cases, and I will file a clarification issue about that, but I think the intent is clear.

Jun 2 2020, 8:47 AM · debug-info, Restricted Project

Jun 1 2020

probinson added a comment to D80876: [clang] Default to windows response files when running on windows.

Re testing, you could copy clang/test/Driver/at_file_win.c, which has an explicit --rsp-quoting=windows; remove that option and make it REQUIRES: system-windows. That should do it.

Jun 1 2020, 12:59 PM · Restricted Project
probinson added a comment to D80876: [clang] Default to windows response files when running on windows.

It looks like your patch will allow us to remove a private patch that has a similar effect.
Incidentally if I apply this to an upstream checkout on Windows, I see clang/test/Driver/at_file.c fails, so you'd at least need to do something for that.
(UNSUPPORTED: system-windows at a minimum.)

Jun 1 2020, 12:27 PM · Restricted Project
probinson updated subscribers of D80876: [clang] Default to windows response files when running on windows.

I was about to add @rnk who I remember has been in Windows driver behavior discussions in the past; except he has cleverly left a note that he's away June-Sept. Oh well.

Jun 1 2020, 11:18 AM · Restricted Project
probinson added a comment to D80876: [clang] Default to windows response files when running on windows.

Indeed I am mis-remembering, sorry about that! We use gnu-style command-line options but we change RSPQuoting from Default to Windows; assuming getProcessTriple() is the host not the target, your change should have the same effect. I'll try removing our private patch and adding yours, and see what happens.

Jun 1 2020, 11:18 AM · Restricted Project
probinson added a comment to D80876: [clang] Default to windows response files when running on windows.

As a functional change it should definitely have a functional test.
Let me double-check on how our stuff behaves, I may be mis-remembering.

Jun 1 2020, 10:46 AM · Restricted Project
probinson added a comment to D80242: [Clang] implement -fno-eliminate-unused-debug-types.

For C++, I'd imagine:

class foo {};
using my_foo = foo;
template <typename bar>
struct baz {
  bar my_bar;
};

but it seems that g++ doesn't emit debug info the for the templated struct.

Jun 1 2020, 9:39 AM · Restricted Project
probinson added inline comments to D80876: [clang] Default to windows response files when running on windows.
Jun 1 2020, 9:39 AM · Restricted Project
probinson accepted D80523: [DebugInfo] Report the format of debug info tables..

LGTM with the suggested helper.

Jun 1 2020, 8:00 AM · debug-info, Restricted Project

May 29 2020

probinson committed rG8c2d2d971b2a: Preserve DbgLoc when DeadArgumentElimination rewrites a 'ret'. (authored by probinson).
Preserve DbgLoc when DeadArgumentElimination rewrites a 'ret'.
May 29 2020, 10:22 AM
probinson closed D80756: Preserve DbgLoc when DeadArgumentElimination rewrites a 'ret'..
May 29 2020, 10:21 AM · debug-info, Restricted Project
probinson added a comment to D80756: Preserve DbgLoc when DeadArgumentElimination rewrites a 'ret'..

Fixed by llvmorg-11-init-16204-g8c2d2d971b2 (forgot to put the revision in the commit log).

May 29 2020, 10:21 AM · debug-info, Restricted Project

May 28 2020

probinson added reviewers for D80756: Preserve DbgLoc when DeadArgumentElimination rewrites a 'ret'.: dblaikie, jmorse.
May 28 2020, 1:45 PM · debug-info, Restricted Project
probinson created D80756: Preserve DbgLoc when DeadArgumentElimination rewrites a 'ret'..
May 28 2020, 1:45 PM · debug-info, Restricted Project
probinson added a project to D80756: Preserve DbgLoc when DeadArgumentElimination rewrites a 'ret'.: debug-info.
May 28 2020, 1:45 PM · debug-info, Restricted Project

May 27 2020

probinson added a comment to D80523: [DebugInfo] Report the format of debug info tables..

I believe GNU readelf and friends tend to put this info after the length, e.g. "Length: 0x1234 (64-bit)" or "Length: 0x1234 (32-bit)" so it might be worth being vaguely similar.

May 27 2020, 12:29 PM · debug-info, Restricted Project

May 18 2020

probinson accepted D79980: [PS4] Enable relaxed relocations by default.

LGTM

May 18 2020, 6:57 AM · Restricted Project

May 13 2020

probinson added a comment to D79868: [DebugInfo] Correct debuginfo for post-ra hoist and sink in Machine LICM.

Can you explain why the sunk instruction's location is considered incorrect? I would have naively assumed that for profiling particularly, you would want to retain the original location to ensure that it is counted.

May 13 2020, 9:44 AM · Restricted Project
probinson accepted D79839: Add shim for fork() on PS4.

LGTM

May 13 2020, 4:48 AM · Restricted Project

May 11 2020

probinson added a comment to D79306: llvm rejects DWARF operator DW_OP_push_object_address..

I can see that DW_OP_push_object_address would be the norm for a Fortran array with a descriptor, but I wouldn't expect it to be used for all subranges. For example a VLA would need to point to a separate entity containing the upper bound, but that separate entity wouldn't be tied to the object address.

May 11 2020, 8:01 AM · Restricted Project, debug-info

Apr 30 2020

probinson added a comment to D79190: llvm rejects DWARF operator DW_OP_lit[1-31] in IR.

If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".

Apr 30 2020, 1:26 PM · Restricted Project, debug-info
probinson accepted D79093: [DebugInfo] Fix printing values of forms which depend on the DWARF format..

LGTM

Apr 30 2020, 7:56 AM · debug-info, Restricted Project

Apr 29 2020

probinson added a comment to D78245: [LIT] Make `%T` unique for every test.

D35396 is pretty long, so as a public service, summarizing the objections to %T from D35396, having lit always pre-create a %T directory would slow down test runs. Considering %T has been deprecated and work done to remove it from llvm, clang, etc, there are not many tests that use it and it would pretty much be an unnecessary penalty for everyone (except libcxx, who needs to create the directory anyway). (I think I got that right.)

Apr 29 2020, 1:28 PM · Restricted Project, Restricted Project, Restricted Project
probinson added a comment to D79093: [DebugInfo] Fix printing values of forms which depend on the DWARF format..

If the value to be printed is only 10 digits wide, I think we do not get 6 leading zeros? Is that how we are formatting 64-bit values in general?

Apr 29 2020, 11:16 AM · debug-info, Restricted Project

Apr 23 2020

probinson added a comment to D78672: [Debuginfo] Remove redundand variable from getAttributeValue().

Thinking from the compiler side: it would be really cool if the compiler could emit all variable sized DWARF attributes at the end of each DIE to maximize how many constant attribute values accesses we could do (if we implement the O(1) access to the value perf optimization in the debug info).

Apr 23 2020, 1:00 PM · debug-info, Restricted Project
probinson added inline comments to D78736: [DWARF5]: Added support for dumping strx forms in llvm-dwarfdump.
Apr 23 2020, 12:29 PM · Restricted Project, debug-info

Apr 17 2020

probinson added a comment to D78024: [FileCheck] - Fix the false positive when -implicit-check-not is used with an unknown -check-prefix..

I'm just reading this review for the first time, and my thought was, why is this interacting with implicit-check-not?

Apr 17 2020, 10:47 AM · Restricted Project, Restricted Project
probinson accepted D78102: [MC][DWARF] Corrected handling of is_stmt flag in .loc directives.

I suggest using past tense in the description, because commentary should describe the code as it is (after the patch).
Ex: llvm assembler handled ... it forced ... is_stmt was specified ...

Apr 17 2020, 5:22 AM · Restricted Project, debug-info

Apr 16 2020

probinson updated subscribers of D78102: [MC][DWARF] Corrected handling of is_stmt flag in .loc directives.

+ llvm-commits

Apr 16 2020, 1:55 PM · Restricted Project, debug-info
probinson added a comment to D78102: [MC][DWARF] Corrected handling of is_stmt flag in .loc directives.

As mentioned above, the commit log should have a short standalone description of the problem/fix. It's fine to cite the bug report as well, but someone doing a "git log" should be able to tell what your patch is about without having to click links or look elsewhere.

Apr 16 2020, 1:55 PM · Restricted Project, debug-info

Apr 8 2020

probinson added a comment to D77393: [X86] Fix implicit sign conversion warnings in X86 headers..

It looks like you're not actually interested in the compiled output, but just whether warnings occurred; in that case you'd be better off with -verify -fsyntax-only and // expected-no-diagnostics.

Apr 8 2020, 7:02 AM · Restricted Project

Apr 6 2020

probinson added inline comments to D77557: [DWARFDebugLine] Use truncating data extractors for prologue parsing.
Apr 6 2020, 1:37 PM · Restricted Project
probinson added inline comments to D77557: [DWARFDebugLine] Use truncating data extractors for prologue parsing.
Apr 6 2020, 8:38 AM · Restricted Project
probinson added a comment to D77438: Add option to limit Debugify to locations (omitting variables).

I know it's a debugging/testing option, but it would be good to have a test that shows the two different levels do what you want.

Apr 6 2020, 8:05 AM · Restricted Project
probinson added a comment to D76878: Implement DW_{OP,AT}_LLVM_* for Heterogeneous Debugging.

Right, I think with so few vendor encodings left there has to be some mechanism for reusing them or opening up the encoding space. Just increasing the vendor extension range will eat into the already dwindling standard encoding space. We also tried really hard to make the entire proposal backwards compatible, and switching to ULEB128 seems like a hard break in compatibility as the 7th bit is already active in DW_OP_'s even in the non-user range. Maybe reserving a single DW_OP_ to say "what follows is a ULEB128 extended-opcode" would be feasible, although it would still have a pretty far reaching impact.

Apr 6 2020, 8:05 AM · debug-info, Restricted Project

Apr 3 2020

probinson committed rG210f40fe9a30: Test had incorrect check for nonzero count (authored by probinson).
Test had incorrect check for nonzero count
Apr 3 2020, 12:59 PM
probinson added a comment to D77227: [RFC][FileCheck] Require colon immediately after CHECK directives.

Grepping for \bCHECK(NEXT|DAG|SAME|NOT): in llvm/test and clang/test (that is, a directive missing the hyphen separator) turns up nothing. Are gotchas 4 and 5 really something we should worry about?

Apr 3 2020, 11:53 AM · Restricted Project