- User Since
- May 9 2013, 11:10 AM (271 w, 7 h)
LGTM but I'll let @JDevlieghere have the last word. It looks like he still has one open question in DwarfDebug.cpp.
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.
@rsmith any further thoughts? We would really like to get this in before the LLVM 7.0 branch, currently scheduled for 1 August.
A few typos.
A bunch of style comments. Maybe try clang-format-diff.
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.
Tue, Jul 17
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.
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).
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.
Mon, Jul 16
Wed, Jul 11
Tue, Jul 10
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.
Mon, Jul 9
Minor drive-by comment.
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.
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.
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.
Still LGTM. And of course still waiting on the other reviews, which I'm doing next.
Some minor things. clang-format-diff is your friend. :-)
Tue, Jul 3
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.
Mon, Jul 2
Sorry, will get back to the FileCheck stuff first thing tomorrow.
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.
Fri, Jun 29
Thu, Jun 28
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.
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?
Wed, Jun 27
Recommend false instead of input.
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.
Do you have commit access or should I commit this for you?
Tue, Jun 26
A couple of naming suggestions but LGTM.
One doc question, otherwise LGTM.
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?
LGTM, thanks for the cleanup!
Fri, Jun 22
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?
You guys duke it out and let me know.
Awesome. I'm fine with changing the advice to use false then. Anything else?
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.
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.
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.
Bowing to popular demand.
Thu, Jun 21
Wed, Jun 20
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.
Moving DBG_VALUE instructions along with the instructions that define the associated values is the right thing to do.
Looks useful, especially given the context of the other patch. LGTM.
Jun 18 2018
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 15 2018
@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 14 2018
Jun 13 2018
Privatize the flags for tracking MD5 usage.
Simplify bool updates.