probinson (Paul Robinson)
User

Projects

User Details

User Since
May 9 2013, 11:10 AM (271 w, 7 h)

Recent Activity

Today

probinson added a comment to D49493: [DebugInfo] Reduce debug_str_offsets section size.

LGTM but I'll let @JDevlieghere have the last word. It looks like he still has one open question in DwarfDebug.cpp.

Thu, Jul 19, 9:21 AM
probinson added a comment to D49493: [DebugInfo] Reduce debug_str_offsets section size.

I agree the ordering test looks fragile. The only other option that comes to mind is trying to exercise the APIs directly from a unittest, but then you need enough scaffolding around it to capture at least the assembler output and verify it. Probably best to leave the test as it is.

Thu, Jul 19, 9:14 AM
probinson added a comment to D46190: For a used declaration, mark any associated usings as referenced..

@rsmith any further thoughts? We would really like to get this in before the LLVM 7.0 branch, currently scheduled for 1 August.

Thu, Jul 19, 7:38 AM · Restricted Project

Yesterday

probinson added a comment to D49493: [DebugInfo] Reduce debug_str_offsets section size.

A few typos.

Wed, Jul 18, 10:56 AM
probinson added a comment to D46190: For a used declaration, mark any associated usings as referenced..

A bunch of style comments. Maybe try clang-format-diff.

Wed, Jul 18, 10:05 AM · Restricted Project
probinson added a comment to D49426: [DEBUG_INFO] fix .loc directives emitted for missing prologues.

I wonder if we should experiment with making the line-table builder complain about a .loc that doesn't increment the address. That would tell us whether these zero-length cases show up anywhere else. Or, if we decide the assembler should handle it correctly, whether anything slips through.

Wed, Jul 18, 9:20 AM
probinson added a comment to D49426: [DEBUG_INFO] fix .loc directives emitted for missing prologues.

However, this particular piece of work addresses what I perceived to be an artefact, a .loc directive for a missing prologue.

I'm not sure it's really a bug here, though - if the assembler addressed the sub-optimality of producing zero-length regions. It seems to me like a natural implementation to change the location, emit the prologue, then change the location again - and if the prologue happens to be empty/zero-length, the correct output falls out naturally. Avoiding special casing the empty prologue keeps the code simple/easier to read/understand, I think.

Wed, Jul 18, 8:54 AM

Tue, Jul 17

probinson added a comment to D49297: expose debugify as an LLVM option in clang.

I will try to look at this tomorrow if @vsk doesn't. Passes are not really my thing but as it's based on the existing opt code it should not be too hard to review.

Tue, Jul 17, 3:26 PM
probinson added inline comments to D49214: [DWARF v5] emit DWARF v5 range lists (no support for fission yet).
Tue, Jul 17, 12:19 PM
probinson added a comment to D49426: [DEBUG_INFO] fix .loc directives emitted for missing prologues.

The repeated address is syntactically valid per the rules for the line-number program, but semantically undefined per the rationale in the non-normative description of the table that the line-number program is supposed to be encoding (i.e., a table indexed by instruction address).

Tue, Jul 17, 10:56 AM
probinson added inline comments to D49214: [DWARF v5] emit DWARF v5 range lists (no support for fission yet).
Tue, Jul 17, 10:53 AM
probinson added inline comments to D49420: [DebugInfo] Generate .debug_names section when it makes sense.
Tue, Jul 17, 10:38 AM
probinson added inline comments to D49214: [DWARF v5] emit DWARF v5 range lists (no support for fission yet).
Tue, Jul 17, 10:25 AM
probinson added inline comments to D49420: [DebugInfo] Generate .debug_names section when it makes sense.
Tue, Jul 17, 8:14 AM
probinson added a comment to D49420: [DebugInfo] Generate .debug_names section when it makes sense.

This is slightly complicated by the fact that the debug_names tables are
incompatible with the DWARF v4 type units (they assume that the type
units are in the debug_info section), and unfortunately, right now we
generate DWARF v4-style type units even for -gdwarf-5.

Tue, Jul 17, 8:07 AM
probinson updated subscribers of D49426: [DEBUG_INFO] fix .loc directives emitted for missing prologues.

+llvm-commits.

Tue, Jul 17, 8:00 AM
probinson updated subscribers of D49328: [FileCheck] Provide an option for FileCheck to dump original input to stderr on failure.

+llvm-commits

Tue, Jul 17, 5:40 AM

Mon, Jul 16

probinson added inline comments to D49214: [DWARF v5] emit DWARF v5 range lists (no support for fission yet).
Mon, Jul 16, 12:37 PM

Wed, Jul 11

probinson committed rL336832: Quick fix for some Windows bots.
Quick fix for some Windows bots
Wed, Jul 11, 11:56 AM
probinson accepted D49192: [FileCheck] Add -allow-deprecated-dag-overlap to failing lldb tests.

LGTM

Wed, Jul 11, 10:48 AM

Tue, Jul 10

probinson committed rL336687: Update test to work on Windows.
Update test to work on Windows
Tue, Jul 10, 8:28 AM
probinson committed rL336685: Support -fdebug-prefix-map for assembler source (pass to cc1as). This.
Support -fdebug-prefix-map for assembler source (pass to cc1as). This
Tue, Jul 10, 8:20 AM
probinson committed rC336685: Support -fdebug-prefix-map for assembler source (pass to cc1as). This.
Support -fdebug-prefix-map for assembler source (pass to cc1as). This
Tue, Jul 10, 8:20 AM
probinson closed D48989: -fdebug-prefix-map option for cc1as.
Tue, Jul 10, 8:20 AM
probinson added a comment to D47106: [FileCheck] Make CHECK-DAG non-overlapping.

The mechanical changes to the tests are actually pretty easy to skim through, given the way Phabricator's web interface does change highlighting. The three you cited are all fine.

Tue, Jul 10, 8:09 AM
probinson accepted D47326: [FileCheck] Add -allow-deprecated-dag-overlap to failing compiler-rt tests.

LGTM

Tue, Jul 10, 8:07 AM
probinson accepted D47172: [FileCheck] Add -allow-deprecated-dag-overlap to failing clang tests.

LGTM

Tue, Jul 10, 8:06 AM
probinson accepted D47171: [FileCheck] Add -allow-deprecated-dag-overlap to failing llvm tests.

LGTM

Tue, Jul 10, 8:02 AM
probinson committed rL336680: Support -fdebug-prefix-map in llvm-mc. This is useful to omit the.
Support -fdebug-prefix-map in llvm-mc. This is useful to omit the
Tue, Jul 10, 7:47 AM
probinson closed D48988: Debug prefix map for machine code emission.
Tue, Jul 10, 7:46 AM

Mon, Jul 9

probinson added a comment to D49102: [AccelTable] Provide DWARF5AccelTableStaticData for dsymutil..

Minor drive-by comment.

Mon, Jul 9, 3:45 PM
probinson added a comment to D49084: FileCheck: Add support for variable expressions.

Maybe this should be a new class of "numeric variables" with distinct syntax, e.g., [[#VAR]] or something, with an implicit regex of [[:digit:]]+ instead of making the test-writer use a valid regex.

Mon, Jul 9, 1:00 PM
probinson accepted D48988: Debug prefix map for machine code emission.

If you have the interest and knowledge to automate clang-format-diff with arcanist, I'm sure there are many people who would be very happy to see a patch. I don't use arcanist but I know others do.

Mon, Jul 9, 12:44 PM
probinson accepted D47106: [FileCheck] Make CHECK-DAG non-overlapping.

Given that you feel so very strongly about it, and that this is a tool that really has absolutely zero exposure outside our own test system, I'm willing to say that erring on the side of extra testing is acceptable here.

Mon, Jul 9, 12:36 PM
probinson added a comment to D47114: [FileCheck] Implement -v and -vv for tracing matches.

Still LGTM. And of course still waiting on the other reviews, which I'm doing next.

Mon, Jul 9, 12:25 PM
probinson added a comment to D48988: Debug prefix map for machine code emission.

Some minor things. clang-format-diff is your friend. :-)

Mon, Jul 9, 10:52 AM
probinson added a comment to D48989: -fdebug-prefix-map option for cc1as.

However, please add a test to ensure that the paths are mapped when invoking the assembler

I added the tests to check the mapping logic through llvm-mc in D48988. In this revision, I merely test if the driver is passing the flags to cc1as. The only thing that is untested is if cc1as_main is setting the options in MCContext correctly. If you want to see a test for that, please can you guide me a little on an appropriate way to test this within tools/clang/test.

Mon, Jul 9, 10:29 AM

Tue, Jul 3

probinson added inline comments to D47106: [FileCheck] Make CHECK-DAG non-overlapping.
Tue, Jul 3, 2:29 PM
probinson added inline comments to D47106: [FileCheck] Make CHECK-DAG non-overlapping.
Tue, Jul 3, 12:44 PM
probinson added a comment to D47106: [FileCheck] Make CHECK-DAG non-overlapping.

Regarding the DAG-NOT-DAG reordering diagnostic: Rereading the comments, and the llvm-dev discussion, are we agreed that we can eliminate it? That is, the first DAG group defines some match range, the second DAG group defines a disjoint match range, and the NOT group looks at the text from the end of the first match range to the start of the second match range. No attempt to diagnose ordering problems.

Tue, Jul 3, 8:58 AM

Mon, Jul 2

probinson added a comment to D47106: [FileCheck] Make CHECK-DAG non-overlapping.

Sorry, will get back to the FileCheck stuff first thing tomorrow.

Mon, Jul 2, 1:51 PM
probinson added a comment to D46850: [DebugInfo] Generate fixups as emitting DWARF .debug_line..

I have spent a little time familiarizing myself with the fragment stuff, which prompts the inline questions.
If you agree with my point about not having two places to store the fragment contents, then I think the first step would be to do that NFC refactoring to change the base class, and follow that with the change to handle the fixups for fixed advance.

Mon, Jul 2, 9:08 AM

Fri, Jun 29

probinson committed rL336007: Pass DWARFUnit to verifier by reference not by value. I am moderately.
Pass DWARFUnit to verifier by reference not by value. I am moderately
Fri, Jun 29, 12:22 PM

Thu, Jun 28

probinson accepted D48730: [DEBUG_INFO, NVPTX] Do not emit .debug_loc section..

Okay. Most compilers don't bother with location lists at -O0, and Clang is among them. So this should be fine for -O0 and won't break your tools at higher optimization. LGTM.

Thu, Jun 28, 2:24 PM
probinson added a comment to D48730: [DEBUG_INFO, NVPTX] Do not emit .debug_loc section..

This seems rather drastic. Nearly all variables will be reported as "optimized away" at any optimization level above -O0.
Have you looked at whether you can identify a single preferred location for the variable?

Thu, Jun 28, 12:47 PM

Wed, Jun 27

probinson committed rL335775: Document the git config for Windows to do line-endings correctly..
Document the git config for Windows to do line-endings correctly.
Wed, Jun 27, 1:03 PM
probinson closed D48494: [doc] Show the git config for Windows to do line-endings correctly.
Wed, Jun 27, 1:03 PM
probinson updated the diff for D48494: [doc] Show the git config for Windows to do line-endings correctly.

Recommend false instead of input.

Wed, Jun 27, 1:01 PM
probinson added a comment to D48640: [SROA] Preserve DebugLoc when rewriting alloca partitions.

Is there an existing test for rewriting the alloca, that you could modify? Instead of adding a whole new test file.
Otherwise seems pretty straightforward. This appears to be a common problem, failing to setDebugLoc after creating a new instruction.

Wed, Jun 27, 6:20 AM
probinson added a comment to D48590: [DwarfDebug] Remove unused argument (NFC).

Do you have commit access or should I commit this for you?

Wed, Jun 27, 6:14 AM · debug-info

Tue, Jun 26

probinson accepted D48408: [Debugify] Diagnose mis-sized dbg.values.

A couple of naming suggestions but LGTM.

Tue, Jun 26, 2:59 PM
probinson accepted D28896: [FileCheck] Add directive for checking for blank lines.

One doc question, otherwise LGTM.

Tue, Jun 26, 7:39 AM
probinson added a comment to D48408: [Debugify] Diagnose mis-sized dbg.values.

Hm. DataLayout is willing to report back one of three possible sizes: the size of a value; the size written by a store; and the stride of an array. getTypeAllocSize() returns the third of these, which would include alignment padding as needed.
Is that really the value size we expect from dbg.value?

Tue, Jun 26, 6:42 AM
probinson accepted D48590: [DwarfDebug] Remove unused argument (NFC).

LGTM, thanks for the cleanup!

Tue, Jun 26, 6:24 AM · debug-info

Fri, Jun 22

probinson added a comment to D48408: [Debugify] Diagnose mis-sized dbg.values.

I remember there being a difference of opinion about the "correct" size of an 80-bit float in a 128-bit container. How would that case be handled here?

Fri, Jun 22, 2:16 PM
probinson added a comment to D48494: [doc] Show the git config for Windows to do line-endings correctly.

You guys duke it out and let me know.

Fri, Jun 22, 1:38 PM
probinson added a comment to D48494: [doc] Show the git config for Windows to do line-endings correctly.

You guys duke it out and let me know.

Fri, Jun 22, 12:55 PM
probinson added a comment to D48494: [doc] Show the git config for Windows to do line-endings correctly.

Awesome. I'm fine with changing the advice to use false then. Anything else?

Fri, Jun 22, 12:35 PM
probinson added a comment to D48494: [doc] Show the git config for Windows to do line-endings correctly.

Personally I've been using the Visual Studio editor. I don't know what other people on the team use. I do know we had some CRLF issues at one point. It has been a long time so I don't remember the details.

Fri, Jun 22, 12:04 PM
probinson added a comment to D48494: [doc] Show the git config for Windows to do line-endings correctly.

So, input is like false on checkout, and fixes crlfs on checkin. The set of test files with actual CRLFs that are required is pretty small, I would think.

Fri, Jun 22, 11:44 AM
probinson added a comment to D48494: [doc] Show the git config for Windows to do line-endings correctly.

I'm sure that I had 50-some clang test failures until I set autocrlf=input and forced a new checkout.
I probably also had the wants-to-commit-the-universe problem but I'm less clear on that; this was 2-3 months ago.

Fri, Jun 22, 11:20 AM
probinson added a comment to D48494: [doc] Show the git config for Windows to do line-endings correctly.

You should use core.autocrlf false.

Fri, Jun 22, 11:08 AM
probinson added a comment to D28896: [FileCheck] Add directive for checking for blank lines.

Bowing to popular demand.

Fri, Jun 22, 10:59 AM
probinson created D48494: [doc] Show the git config for Windows to do line-endings correctly.
Fri, Jun 22, 10:38 AM
probinson committed rL335356: Fix test again, try to keep all targets happy.
Fix test again, try to keep all targets happy
Fri, Jun 22, 8:24 AM
probinson committed rL335353: Fix test, nop is not always 1 byte.
Fix test, nop is not always 1 byte
Fri, Jun 22, 8:12 AM
probinson committed rL335350: [DWARFv5] Allow ".loc 0" to refer to the root file..
[DWARFv5] Allow ".loc 0" to refer to the root file.
Fri, Jun 22, 7:20 AM
probinson closed D48452: [DWARFv5] Allow ".loc 0" to refer to the root file.
Fri, Jun 22, 7:20 AM · debug-info

Thu, Jun 21

probinson added inline comments to D48452: [DWARFv5] Allow ".loc 0" to refer to the root file.
Thu, Jun 21, 2:25 PM · debug-info
probinson added inline comments to D48452: [DWARFv5] Allow ".loc 0" to refer to the root file.
Thu, Jun 21, 2:21 PM · debug-info
probinson created D48452: [DWARFv5] Allow ".loc 0" to refer to the root file.
Thu, Jun 21, 1:48 PM · debug-info
probinson committed rL335254: [DWARF] Warn on and ignore ".file 0" for DWARF v4 and earlier..
[DWARF] Warn on and ignore ".file 0" for DWARF v4 and earlier.
Thu, Jun 21, 9:46 AM

Wed, Jun 20

probinson added a comment to D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.

As I had noted earlier, once you modify the pass so that the debug instruction doesn't interfere with the code motion, the associated debug instructions have to move with the real instruction. In that respect, the patch is doing the right thing.

Wed, Jun 20, 3:19 PM · debug-info
probinson added a comment to D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.

If we were to reorder the instruction stream so that the two intrinsics are swapped, it will look like the value of p is NULL throughout the rest of the program.

Wed, Jun 20, 12:31 PM · debug-info
probinson added a comment to D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.
  • the optimizer should never move a debug intrinsic over another intrinsic
Wed, Jun 20, 10:49 AM · debug-info
probinson added a comment to D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.

Moving DBG_VALUE instructions along with the instructions that define the associated values is the right thing to do.

Wed, Jun 20, 10:44 AM · debug-info
probinson committed rL335146: [DWARF] Don't keep a ref to possibly stack allocated data..
[DWARF] Don't keep a ref to possibly stack allocated data.
Wed, Jun 20, 10:13 AM
probinson added a comment to D47720: [DebugInfo] Inline for without DebugLocation.

ping! should I commit this?

Wed, Jun 20, 9:38 AM
probinson committed rL335143: Remove a redundant initialization. NFC.
Remove a redundant initialization. NFC
Wed, Jun 20, 9:16 AM
probinson added inline comments to D48331: [DebugInfo][InstCombine] Preserve DI after combining zext instructions.
Wed, Jun 20, 7:17 AM
probinson accepted D48340: [Local] Add a utility to insert replacement dbg.values, NFC.

Looks useful, especially given the context of the other patch. LGTM.

Wed, Jun 20, 6:50 AM

Jun 18 2018

probinson added a comment to D46190: For a used declaration, mark any associated usings as referenced..

Style comments.
The two new Sema methods (for namespaces and using references) are C++ specific, so SemaDeclCXX.cpp would seem like a more appropriate home for them.

Jun 18 2018, 8:50 AM · Restricted Project
probinson committed rUNW334936: Update copyright year to 2018..
Update copyright year to 2018.
Jun 18 2018, 8:02 AM
probinson closed D48219: Update copyright year in license files.

And patch lgtm. I don't see any reason why it would interact with the re-licensing really.

Jun 18 2018, 5:35 AM
probinson committed rCTE334936: Update copyright year to 2018..
Update copyright year to 2018.
Jun 18 2018, 5:30 AM
probinson committed rLLD334936: Update copyright year to 2018..
Update copyright year to 2018.
Jun 18 2018, 5:28 AM
probinson committed rCXXA334936: Update copyright year to 2018..
Update copyright year to 2018.
Jun 18 2018, 5:27 AM
probinson committed rCRT334936: Update copyright year to 2018..
Update copyright year to 2018.
Jun 18 2018, 5:27 AM
probinson committed rL334936: Update copyright year to 2018..
Update copyright year to 2018.
Jun 18 2018, 5:27 AM
probinson committed rC334936: Update copyright year to 2018..
Update copyright year to 2018.
Jun 18 2018, 5:26 AM

Jun 15 2018

probinson added a reviewer for D48219: Update copyright year in license files: hans.

@hans maybe this task should go on the release manager checklist? That's the closest thing to an annual reminder I can think of, that is likely to work.

Jun 15 2018, 7:33 AM
probinson created D48219: Update copyright year in license files.
Jun 15 2018, 7:27 AM

Jun 14 2018

probinson committed rL334710: [DWARFv5] Tolerate files not all having an MD5 checksum..
[DWARFv5] Tolerate files not all having an MD5 checksum.
Jun 14 2018, 6:43 AM
probinson closed D48135: [DWARFv5] Tolerate files not all having an MD5 checksum..
Jun 14 2018, 6:43 AM · debug-info
probinson added a comment to D46190: For a used declaration, mark any associated usings as referenced..

@dblaikie is @rsmith back from the standards meeting yet? I hate to be a pest but this is blocking other work Carlos has in progress.

Jun 14 2018, 5:31 AM · Restricted Project

Jun 13 2018

probinson added inline comments to D48135: [DWARFv5] Tolerate files not all having an MD5 checksum..
Jun 13 2018, 12:27 PM · debug-info
probinson updated the diff for D48135: [DWARFv5] Tolerate files not all having an MD5 checksum..

Privatize the flags for tracking MD5 usage.

Jun 13 2018, 12:24 PM · debug-info
probinson added inline comments to D48135: [DWARFv5] Tolerate files not all having an MD5 checksum..
Jun 13 2018, 11:11 AM · debug-info
probinson updated the diff for D48135: [DWARFv5] Tolerate files not all having an MD5 checksum..

Simplify bool updates.

Jun 13 2018, 11:10 AM · debug-info
probinson created D48135: [DWARFv5] Tolerate files not all having an MD5 checksum..
Jun 13 2018, 9:53 AM · debug-info