A rebase shouldn't affect the spirit of the change :-)
Fri, Jun 21
Thu, Jun 20
@aprantl The comment looks OK? :)
@aprantl This still looks OK after the rebasing? :)
Sounds alright to me
-Add the comment explaining the assertion when deleting instructions
Wed, Jun 19
- fix alignment of pointer in inalloca case
- make existing tests stop failing by changing some and adding a check for existing return value alloca (I think?) before adding the ReturnValuePointer alloca
We do not precisely match gcc/gas behavior in some more-peculiar cases, but my assertion is that those should not occur "naturally" and so it's okay. For example:
foo: nop .file 1 "a.c"
This will cause gcc/gas to emit an error to the effect that file number 1 is already defined (implicitly, because of the line-table record for the first instruction). Clang/llvm-mc will not, we'll just emit an odd-looking line table. I can see how to cause clang/llvm-mc to emit this error, but it feels like it would be a hack done just to match gcc's (likely unintentional) error-detection behavior for an ill-formed assembler file.
I see we don't have any tests for inalloca to model this on, so I think we should skip that for this change. I'll add one later that handles arguments as well, since those are interesting.
- Add clang and llvm tests, and windows debuginfo test
- Use GEP as returnValuePointer in inalloca case
- Add bool parameter for EmitDeclare when pointer is being used
Thanks for your help!
Thanks, I've now resubmitted this patch (1251cac62af5).
Tue, Jun 18
LGTM. Thanks for doing this!
Thanks, this addresses my concern!
This needs two unit tests:
- A clang test at clang/test/CodeGenCXX/debug-info-nrvo.cpp similar to other debug-info-* tests there. This test should have a second RUN line and extra checks for -fno-elide-constructors.
- An LLVM test at llvm/test/DebugInfo/COFF/nrvo.ll to show that we handle dbg.declare + deref
- add semicolon
Now creates a pointer to the return location in the function prolog, whenever sret is being used.
I put up a Verifier patch at https://reviews.llvm.org/D63499. Once that patch has landed, this patch should be safe to re-apply.
Mon, Jun 17
As I mentioned in PR38449, I plan to look at this starting this week. Even benign situations such as
foo: .file 1 "a.c"
are tripping over the assertion. I think the correct tactic is not to remove the places that are doing the checks, but make those places do a better job of tidying up anything that had been done in response to the command-line -g in favor of allowing the embedded directives to DTRT.
This seems to be a case of having old, incorrect, metadata baked into the test.
The 'startLoc' (!680) and 'endLoc' (!681) in loop metadta for loop !679 are both
missing an 'inlinedAt' node.
Fri, Jun 14
- fix test case
Ping. Do we need to explain our concern more precisely?
Thu, Jun 13
Use a debug loc with line 0 instead, fix a test that should have been using fastisel.