Page MenuHomePhabricator

[DebugInfo] Emit locations for large constant integers

Authored by labath on Nov 6 2020, 2:04 AM.



This adds support for emissing of integers larger than 8 bytes (e.g
__int128_t). It reuses/extends the existing code for emitting floating
point constant, using DW_OP_implicit_value (if debugger tuning permits
it), or glueing the value together using DW_OP_const and DW_OP_piece.

Diff Detail

Unit TestsFailed

450 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

labath created this revision.Nov 6 2020, 2:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2020, 2:04 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
labath requested review of this revision.Nov 6 2020, 2:04 AM

Thanks for the patch! Overall this looks nice. We can also represent long double(128 bits) using DW_OP_implicit_value, any plans for supporting that too ? long double is non-trivial like it has different representation on Power. It's pending on me since a while :(

206 ↗(On Diff #303377)

GDB also supports this. When I introduced this representation, I kept it exclusively GDB. Looking at DwarfDebug.cpp:2477 looks like someone changed it along the way.
Do you mind adding GDB also ?

SouraVX added inline comments.Nov 6 2020, 4:43 AM
245 ↗(On Diff #303377)
dblaikie added inline comments.Nov 6 2020, 11:29 AM

What's this change doing? It looks new & not sure what the old code would've been doing previously under this condition (isConstantInt)

202 ↗(On Diff #303377)

This change looks like it'd have two effects:

  1. existing callers of addUnsignedConstant now might get DW_OP_implicit_values where they didn't before
  2. the new call from addConstantFP might lead to that use case getting DW_OP_const values it didn't get before

Could those two situations be separated into distinct patches with distinct tests? (I mean, could be the same test file, but making it clear how adding this new functionality to addUnsignedConstant impacts existing callers, then separately how changing addConstantFP to use addUnsignedConstant changes that)

233 ↗(On Diff #303377)

Is this a separate bugfix? Could go in a separate patch/test/etc?

labath added a comment.Nov 9 2020, 2:07 AM

Thanks for the patch! Overall this looks nice. We can also represent long double(128 bits) using DW_OP_implicit_value, any plans for supporting that too ? long double is non-trivial like it has different representation on Power. It's pending on me since a while :(

Um... maybe? What's the deal with long double and PPC ? Does bitcastToAPInt return a different representation than what's actually used on the machine?


This is the part which actually implements this feature. :)
I don't understand why, but integers <= 64 bit are stored differently than integers with larger width. So, smaller integers would be caught by the isInt check on line 2451, but the code wouldn't do anything with larger integers (AFAICT).

202 ↗(On Diff #303377)

I elaborate on (1) below. You're right about (2). I did check that gdb&lldb can actually handle the expanded usage of DW_OP_const, but I was too lazy to turn it into a test. I'll try to split that off into a separate patch.

206 ↗(On Diff #303377)

This already covers gdb (note the negation). The condition was changed from GDB to not SCE when lldb support for this was added.

233 ↗(On Diff #303377)

I'm not sure I would call it a bugfix, as the function had only one caller (and a pretty strange one at that) and was only being called for size <= 64. However, it is required to reflect the new realities of how this function is used now.

Not emitting DW_OP_stack_value here was kind of correct, as its only caller (DwarfDebug.cpp:2484) was falling through (after ensuring that the size is <= 64) to DwarfExpr.addExpression(std::move(ExprCursor));, which would end up adding DW_OP_stack_value.

I'm not really sure what addExpression does (I am quite new to this area), but in the examples I've tried, it only appends DW_OP_stack_value. I suppose it might be possible to modify the value of the float constant via some dwarf expressions, but that would be fairly tricky as the dwarf operands have integral semantics.

In the new setup, I don't even bother calling addExpression, as hooking that up would be tricky (and of dubious value), as that function could be called on single-piece non-implicit_value expressions. That means this function always needs to emit DW_OP_stack_value itself.

245 ↗(On Diff #303377)

In my mind, the FIXME made the body non-trivial. But I guess it is possible to move that outside the body...

labath marked 4 inline comments as done.Nov 9 2020, 2:28 AM
labath added inline comments.
202 ↗(On Diff #303377)

Created a separate refactoring patch here:

On second though, I don't think there's anything additional to test (until we support long double constants that is). The function was only being called for values smaller than 64 bits, and the existing implict_value FP tests have an SCE test which confirms that DW_OP_stack value is emitted in the same way as before.

labath updated this revision to Diff 303794.Nov 9 2020, 2:31 AM

Rebase on top of the other patch.

dblaikie accepted this revision.Nov 17 2020, 6:05 PM

Looks good!

This revision is now accepted and ready to land.Nov 17 2020, 6:05 PM