Page MenuHomePhabricator

[DebugInfo] Add option to disable inline line tables.
AcceptedPublic

Authored by akhuang on Wed, Sep 18, 10:46 AM.

Details

Summary

This adds a clang option to disable inline line tables. When it is used,
the inliner uses the call site as the location of the inlined function instead of
marking it as an inline location with the function location.

See https://bugs.llvm.org/show_bug.cgi?id=42344

Event Timeline

akhuang created this revision.Wed, Sep 18, 10:46 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptWed, Sep 18, 10:46 AM
rnk added reviewers: jmorse, probinson.

+ other debug info people

llvm/docs/LangRef.rst
1444

This is a string attribute, so it should have quotes around it.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1425

Let's check hasFnAttribute out of the loop so we aren't doing string hash lookups in a loop.

1431–1445

Let's actually try to reuse this !CalleeHasDebugInfo code path when this function attribute is present. They should do the same thing.

As per the bug - I'm not super inclined towards a function attribute here (& even if it's going to be an inliner rather than debug info generation/backend thing (which I'm more on the fence about - think I'm happy with either direction there, really) - I'd still be inclined towards it living in the DICompileUnit with other debug info things, rather than a function attribute). But happy to hear what other folks (especially the usual debug info cabal - @aprantl @probinson @JDevlieghere etc) think.

akhuang updated this revision to Diff 220756.Wed, Sep 18, 3:01 PM
akhuang marked 3 inline comments as done.
  • Address comments
jmorse added a comment.Wed, Oct 2, 8:55 AM

Looks good, although I'm not familiar enough with frontend things to approve IMO.

Using a function attribute seems fine and appropriate too -- although CU flags is another thing I'm unfamiliar with, so I can't really offer an opinion.

aprantl requested changes to this revision.Wed, Oct 2, 9:10 AM

I don't think the implementation is correct, see inline comments.

clang/include/clang/Driver/Options.td
1963

As a DWARF person, this option name is a little confusing since in DWARF inline info is part of debug info, not the line table, but few end-users would actually know. I would probably have called it -gno-inline-info or -gno-inlined-functions. I don't have strong feelings about it though.

llvm/docs/LangRef.rst
1445

Same comment for the attribute.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1426

This will probably cause some IR Verifier failures and very confusing debug info when inlining dbg.value intrinsics. The correct thing to do here is probably to assign line 0 to the inlined instructions and remove all debug info intrinsics. Otherwise the inlined variables will show up in the parent frame, which will screw up debugging.

This revision now requires changes to proceed.Wed, Oct 2, 9:10 AM
aprantl added inline comments.Wed, Oct 2, 9:10 AM
llvm/lib/Transforms/Utils/InlineFunction.cpp
1426

cf. getMergedLocation() for how to do this.

There also needs to be a IR-based test for the inliner change.

rnk added inline comments.Thu, Oct 3, 3:15 PM
clang/include/clang/Driver/Options.td
1963

The other two options we have that control this stuff are -gmlt / -gline-tables-only. gmlt stands for "g minimal line tables". So, our command line interface talks about "line tables" already, and IMO we should stick with it, even if it's not really a table after all.

And, technically, this option will greatly affect the .debug_line section. The inlined source locations are normally present in .debug_line, and this change suppresses them. Instead, the debugger will appear to be stopped at the inlined call site.

llvm/docs/LangRef.rst
1445

Maybe we could use more precise wording rather than talking about tables. Maybe we should describe what happens from the user perspective, something like:

When this attribute is present and set to true, the inliner will discard source locations while inlining code into the current function. Instead, the source location of the call site will be used for all inlined code. Breakpoints set on code that was inlined into the current function will not fire during the execution of the inlined call sites. If the debugger stops inside an inlined call site, it will appear to be stopped at the outermost inlined call site.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1426

Ah, yes, we should erase all debug info for inlined variables while inlining in this mode.

1431–1445

This suggestion makes less sense in light of the need to remove variable information. Use your best judgement.

akhuang updated this revision to Diff 224681.Fri, Oct 11, 2:26 PM
akhuang marked 2 inline comments as done.

-Remove intrinsics debug info
-Add inliner test
-Add to function attribute description

rnk added inline comments.Fri, Oct 11, 2:46 PM
llvm/lib/Transforms/Utils/InlineFunction.cpp
1427

Each of these inherit from DbgVariableIntrinsic, so you should be able to dyn_cast to that, and handle them all with one if.

llvm/test/Transforms/Inline/no-inline-line-tables.ll
32

Test looks good

akhuang updated this revision to Diff 224687.Fri, Oct 11, 2:58 PM
  • Remove extra ifs.
rnk added a comment.Fri, Oct 11, 3:16 PM

I guess the commit message shouldn't say "[CodeView] Add option to disable inline line tables." It's really an option for all debug info. You could put "[DebugInfo]" on there, or just drop the tag.

aprantl accepted this revision.Fri, Oct 11, 3:26 PM

I would still prefer no-inline-info or no-inline-debuginfo over no-inline-linetables and a line 0 location for the inlined instructions. Other than that the patch is now safe.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1431

I still think an artificial (line 0) location would be less misleading for debuggers, profilers, and optimization remarks.

This revision is now accepted and ready to land.Fri, Oct 11, 3:26 PM
akhuang retitled this revision from [CodeView] Add option to disable inline line tables. to [DebugInfo] Add option to disable inline line tables..Fri, Oct 11, 3:26 PM
akhuang updated this revision to Diff 224697.Fri, Oct 11, 3:27 PM
akhuang marked an inline comment as done.

Fix code so that -gno-inline-line-tables works when not codeview

akhuang updated this revision to Diff 224711.Fri, Oct 11, 4:49 PM
  • Set location to line 0 with getMergedLocation
akhuang updated this revision to Diff 224888.EditedMon, Oct 14, 11:58 AM

Fixed to remove all DbgInfoIntrinsics instead of just DbgVariableIntrinsics

rnk added a comment.Mon, Oct 14, 1:10 PM

Based on what we learned in https://llvm.org/PR43530, I think we still want to use the location of the call site and not line zero. :(

llvm/lib/Transforms/Utils/InlineFunction.cpp
1431

That will cause problems for us in practice. There's discussion about this in D68747. Since that change, we treat line zero the same as "no location". If there are no locations in a basic block, then the whole block inherits the line number from the block layout predecessor, which could be unrelated. Keeping the inlined call site location gives us the highest likelihood that "step over" will stop at the next statement. Widely applying line zero to entire basic blocks will put us in that situation more often. We could certainly write a pass to backfill better source locations, but it seems preferable to not put ourselves in that position in the first place.

However, the effect you mention on profilers and optimization remarks is real and concerning. Users should have the power to work around it by removing the flag that applies this attribute, which makes me feel like we should go forward with this as is. If this develops into a real usability problem, we can leave the attribute as is and move the implementation into the backend.

akhuang updated this revision to Diff 224909.Mon, Oct 14, 2:51 PM
  • Fixes for DbgInfoIntrinsic type and change test cmd

Reverted the line 0 change - I wasn't sure if it would be an issue since
the debugger doesn't step through those lines.

rnk added inline comments.Mon, Oct 14, 3:38 PM
llvm/lib/Transforms/Utils/InlineFunction.cpp
1428

Will this work if the dbg.value is the first instruction of a basic block? I'd expect eraseFromParent to return a new iterator pointing to FI->begin(), then operator-- to back up to "before begin", which would probably crash or assert. This would make a good test case and shouldn't be too hard. You can try inlining foo in this example:

void bar();
int foo(bool cond, int x) {
  if (cond) {
    x = 42; // should set up a dbg.value at BB start
    bar(); // block select formation
  }
  return x;
}

Since that change, we treat line zero the same as "no location". If there are no locations in a basic block, then the whole block inherits the line number from the block layout predecessor, which could be unrelated. Keeping the inlined call site location gives us the highest likelihood that "step over" will stop at the next statement.

Who is "we" in this context? The CodeView backend?
As far as DWARF is concerned (and LLVM mostly inherits the DWARF semantics) line 0 is well-defined and means compiler-generated code or otherwise no unambiguous source location. DWARF-based debuggers know to skip over instructions with line 0.

Is the problem that CodeView doesn't have this concept, or does the Windows debugger no know how to deal with it (or both)?

I'm feeling rather strongly that that LLVM should not be emitting wrong debug info to work around bugs in a debugger. I understand that sometimes this isn't possible because we don't control the consumers. The correct thing to do here is to guard the workaround by a debugger tuning flag. For DWARF, we do want line 0 here.

Since that change, we treat line zero the same as "no location". If there are no locations in a basic block, then the whole block inherits the line number from the block layout predecessor, which could be unrelated. Keeping the inlined call site location gives us the highest likelihood that "step over" will stop at the next statement.

Who is "we" in this context? The CodeView backend?
As far as DWARF is concerned (and LLVM mostly inherits the DWARF semantics) line 0 is well-defined and means compiler-generated code or otherwise no unambiguous source location. DWARF-based debuggers know to skip over instructions with line 0.

Is the problem that CodeView doesn't have this concept, or does the Windows debugger no know how to deal with it (or both)?

I'm feeling rather strongly that that LLVM should not be emitting wrong debug info to work around bugs in a debugger. I understand that sometimes this isn't possible because we don't control the consumers. The correct thing to do here is to guard the workaround by a debugger tuning flag. For DWARF, we do want line 0 here.

(+1 to all that, FWIW)

Though I think in this case since it's got to be handled during the transformation (rather than as an after the fact choice at debug-info-emission time) it might not be practical to guard by a debugger tuning flag. It could/should be guarded though, but may just have to be guarded by the format (not that we have any other debuggers consuming CodeView anyway, so I think it's sufficient here).

rnk added a comment.Mon, Oct 14, 5:05 PM

Who is "we" in this context? The CodeView backend?

Yes, the CodeView backend, sorry for the ambiguity.

As far as DWARF is concerned (and LLVM mostly inherits the DWARF semantics) line 0 is well-defined and means compiler-generated code or otherwise no unambiguous source location. DWARF-based debuggers know to skip over instructions with line 0.

Is the problem that CodeView doesn't have this concept, or does the Windows debugger no know how to deal with it (or both)?

Visual Studio in particular has been reported to have problems with line zero. It seems to treat it as some kind of error condition (no line available), and kicks the user over to the assembly view.

I'm feeling rather strongly that that LLVM should not be emitting wrong debug info to work around bugs in a debugger. I understand that sometimes this isn't possible because we don't control the consumers. The correct thing to do here is to guard the workaround by a debugger tuning flag. For DWARF, we do want line 0 here.

I don't think we want to emit line zero here. The use case for this flag is to allow the user to ask the compiler to attribute code from inlined call sites to the call site itself. Maybe the user doesn't want to see the details of push_back.

I actually went ahead and experimented with how gdb handles line zero. I compiled the following program like so:

$ cat t.cpp
volatile int x;
static inline void foo() {
  ++x;
  ++x;
}
int main() {
  ++x;
  foo();
  ++x;
  foo();
  ++x;
  return x;
}
$ clang -g -O2 t.cpp -S -emit-llvm -o t.ll
$ sed -e 's/DILocation.line:.*column:.*, scope\(.*inlinedAt: .*\))/DILocation(line: 0, column: 0, scope\1)/' -i t.ll
$ clang -g t.ll -o t.exe
$ gdb --args t.exe
...

When I step through main with s in gdb, it stops on the ++x lines, and skips the foo() lines completely. I don't think that's the desired behavior, the desired behavior is to treat the body of foo as a single line.

To give more context, back in 2013, @probinson asked if we should add a similar feature here:
http://lists.llvm.org/pipermail/cfe-dev/2013-November/033765.html
This was around the time that inlinedAt locations were being sorted out, I think. His proposed name for this flag was -gno-inlined-scopes. I believe nothing ever came of that discussion, and we continued on our way until today in 2019, when one of our users requested a similar feature. I vaguely recalled the discussion from 2013, but I had forgotten the details, so I figured that it might be a generally useful feature that others would appreciate. So, I suggested that @akhuang add a real flag for it, and that it should work for DWARF and CodeView. That's part of why I figured it would be good to implement it in the inliner, so we don't have to do the work twice.

Given the behavior of gdb shown above, I don't think either set of users, Chromium developers or Sony licensees, have a use case for a flag that applies line zero to inlined functions. I don't think that's what they are asking for. Paul outlined what users actually asked for back then here:
http://lists.llvm.org/pipermail/cfe-dev/2013-November/033782.html

I think users are asking for a flag that attributes the code that the inliner clones to the call site. And, I can't see any reason not to give it to them. Does that seem reasonable?

akhuang updated this revision to Diff 225104.Tue, Oct 15, 1:18 PM
akhuang marked an inline comment as done.

Address comment about bad decrementing iterator.

Apologies for missing this until now. Our email system keeps dropping stuff sent by Phabricator.

FTR, since @rnk has mentioned my years-ago writings, what Sony has internally nowadays is a little different than what I said back then. We have an option spelled -gno-inlined-scopes which is slightly tricky to describe precisely, but the intent is that for debug-info purposes, certain functions appear to be empty. That is, the declaration is still emitted (which is different from nodebug) but the generated IR has no source locations.

We implement this in CodeGenFunction::GenerateCode, rather than down in LLVM. We found that the in-LLVM implementation would affect exactly those functions that were actually inlined by the optimizer, rather than those that had some kind of "inline me" indication in the source. This caught too many cases where they compiler inlined a function because of a cost heuristic rather than a source indication, and made the set of functions with no source locations somewhat unpredictable.

What is an "inline me" indication? The always_inline attribute, the inline keyword, or a class method whose definition is in-class.

Which functions are affected? Everything with always_inline regardless of optimization level, and also anything to which optimization applies and is marked inline or is defined in-class. ("Optimization applies" means compiling at -O1 or higher, and the function does not have OptimizeNoneAttr.)

This might all sound overly complicated, but after (I think) three attempts, it appears to filter out the set of functions that can be considered too small to bother with, and/or well-debugged and unlikely to be the cause of a problem, and of course everything in STL which by nature pretty much all has to have in-class definitions. It's a heuristic that is (at least to an extent) under the control of the programmer, and is a rule based strictly on source annotations and command-line options, rather than arbitrary choices made by the compiler (which possibly vary from build to build).

I haven't taken the time to read through all the prior comments and re-read the PR etc, but I did want to report what it is that we actually do now, and which seems to keep the users-who-care happy enough over the past few years.