This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add TargetInstrInfo::getCalleeOperand
ClosedPublic

Authored by aheejin on May 22 2021, 11:08 PM.

Details

Summary

DwarfDebug unconditionally assumes for all call instructions the 0th
operand is the callee operand, which seems to be true for other targets,
but not for WebAssembly. This adds TargetInstrInfo::getCallOperand
method whose default implementation returns getOperand(0) and makes
WebAssembly overrides it to use its own utility method to get the callee
operand.

Edit:
This also fixes an existing bug in WebAssembly::getCalleeOp, which was
uncovered by this CL.

Diff Detail

Event Timeline

aheejin created this revision.May 22 2021, 11:08 PM
aheejin requested review of this revision.May 22 2021, 11:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2021, 11:08 PM
aheejin added inline comments.May 22 2021, 11:16 PM
llvm/test/CodeGen/WebAssembly/stackified-debug.ll
2 ↗(On Diff #347235)

Without this patch this fails, because

%5:i32 = CALL @input, ...

In this wasm MIR instruction, the callee is the 1th operand, but the current code will incorrectly assume the 0th operand, which is the dest register (%5 here), as a callee, and will try to print its register name, when --debug is given. This crashes because wasm does not use physical registers.

Thanks!

llvm/test/CodeGen/WebAssembly/stackified-debug.ll
2 ↗(On Diff #347235)

I'd prefer a dedicated (standalone) test for this.

dschuff accepted this revision.May 24 2021, 11:33 AM

I think this makes sense.

my extra comment was that it seems likely that there are other places that currently use getOperand(0) that should now use this, and I was wondering if there were an easy way to find them since we can't just grep (especially since LLVM IR also has a getOperand). Thinking about that some more: Presumably uses in other backends would be safe to leave in place, and anything that affects codegen (at least in code that is covered at all) would be pretty broken, so maybe there actually aren't that many places outside of debug info (in other words broken debug info would be much more likely to go unnoticed than broken codegen).

This revision is now accepted and ready to land.May 24 2021, 11:33 AM

I think this makes sense.

my extra comment was that it seems likely that there are other places that currently use getOperand(0) that should now use this, and I was wondering if there were an easy way to find them since we can't just grep (especially since LLVM IR also has a getOperand). Thinking about that some more: Presumably uses in other backends would be safe to leave in place, and anything that affects codegen (at least in code that is covered at all) would be pretty broken, so maybe there actually aren't that many places outside of debug info (in other words broken debug info would be much more likely to go unnoticed than broken codegen).

I tried to search words like CalleeOp in the codebase but all other usages are in target-specific code, and there are only a few of them anyway. Also I think in other places it would have pretty much caused crashes anyway, so I'm not worried too much about it.

aheejin updated this revision to Diff 347517.May 24 2021, 3:19 PM

New standalone test

aheejin updated this revision to Diff 347518.May 24 2021, 3:21 PM

Comment fix

llvm/test/CodeGen/WebAssembly/stackified-debug.ll
2 ↗(On Diff #347235)

I added test/DebugInfo/WebAssembly/call-site.ll and deleted this --debug line. PTAL.

llvm/test/DebugInfo/WebAssembly/dbg-value-list.ll
36 ↗(On Diff #347517)

This is just a drive-by name fix and unrelated to this CL

aheejin added inline comments.May 24 2021, 3:22 PM
llvm/test/DebugInfo/WebAssembly/call-site.ll
9

Without the fix in this CL, this "foo" is not correctly displayed.

dschuff added inline comments.May 24 2021, 5:00 PM
llvm/test/DebugInfo/WebAssembly/call-site.ll
9

Thanks, this is also great because I don't know if we have any other tests that specifically cover callsite info for wasm anyway.

aheejin updated this revision to Diff 347589.May 24 2021, 11:40 PM

Fix WebAssembly::getCalleeOp for indirect calls

This happened to uncover a long-existing bug in WebAssembly::getCalleeOp. For indirect calls it returns MI.getOperand(MI.getNumOperands() - 1), but in the presence of implicit operands, most of the time it returns an incorrect operand. For example:

%37:i32 = CALL_INDIRECT 0, 0, %55:i32, %38:i32, %54:i32, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def $value_stack, implicit $value_stack, debug-location !39; system/lib/libc/musl/src/stdlib/bsearch.c:9:10

The callee operand is %54 here, but this returns implicit $value_stack. The should have used MI.getNumExplicitOperand() instead. This has not been a problem so far because this utility function has been used only from a few places and all of them only cared about direct calls.

This fixes this bug and adds a test for call_indirect too.

aheejin edited the summary of this revision. (Show Details)May 24 2021, 11:50 PM

LGTM (with the comments addressed)

llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp
94

By doing this, the patch does a bit more then described in the summary. I think this is should be a separate change.

llvm/test/DebugInfo/WebAssembly/call-site.ll
4

Super nit: The highlevel comments starts with double ;;

typo: information

37

super nit: producer could be "clang version 11.0.0" only

llvm/test/DebugInfo/WebAssembly/dbg-value-list.ll
36 ↗(On Diff #347517)

This should be a separate change.

aheejin marked 3 inline comments as done.May 25 2021, 12:02 AM
aheejin added inline comments.
llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp
94

I thought about making it a separate change, but it is hard.

  • If we land DwarfDebug fix first, it will break all programs with indirect calls and debug info without this getCalleeOp fix.
  • If we land getCalleeOp fix first, there is no way to test that change because DwarfDebug is currently the only place we use getCalleeOp for indirect calls.

By the way, I fixed the summary to include this fix.

aheejin updated this revision to Diff 347592.May 25 2021, 12:02 AM
aheejin edited the summary of this revision. (Show Details)

Address comments

djtodoro added inline comments.May 25 2021, 12:09 AM
llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp
94

OK, thanks

llvm/test/DebugInfo/WebAssembly/call-site.ll
23

why don't we have here an additional CHECK for the register that holds the indirect call (DW_AT_GNU_call_site_target)?

aheejin added inline comments.May 25 2021, 12:19 AM
llvm/test/DebugInfo/WebAssembly/call-site.ll
23

It doesn't have it. Am I doing something wrong? This is the full llvm-dwarfdump output for this file.

call-site.o:  file format WASM                                                   
                                                                                 
.debug_info contents:                                                            
0x00000000: Compile Unit: length = 0x00000068, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x04 (next unit at 0x0000006c)
                                                                                 
0x0000000b: DW_TAG_compile_unit                                                  
              DW_AT_producer  ("clang version 11.0.0")                           
              DW_AT_language  (DW_LANG_C99)                                      
              DW_AT_name  ("test.c")                                             
              DW_AT_stmt_list  (0x00000000)                                      
              DW_AT_comp_dir  ("/home/llvm-project")                             
              DW_AT_GNU_pubnames  (true)                                         
              DW_AT_low_pc  (0x00000000)                                         
              DW_AT_ranges  (0x00000000                                          
                 [0x00000002, 0x0000000a)                                        
                 [0x0000000b, 0x0000001a))                                       
                                                                                 
0x00000026:   DW_TAG_subprogram                                                  
                DW_AT_low_pc  (0x00000002)                                       
                DW_AT_high_pc  (0x0000000a)                                      
                DW_AT_frame_base  (DW_OP_WASM_location 0x3 0x0, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites  (true)                                 
                DW_AT_name  ("call_direct")                                      
                DW_AT_decl_file  ("/home/llvm-project/test.c")                   
                DW_AT_decl_line  (3)                                             
                DW_AT_external  (true)                                           
                                                                                 
0x0000003d:     DW_TAG_GNU_call_site                                             
                  DW_AT_abstract_origin  (0x00000047 "foo")                      
                  DW_AT_low_pc  (0x00000009)                                     
                                                                                 
0x00000046:     NULL                                                             
                                                                                 
0x00000047:   DW_TAG_subprogram                                                  
                DW_AT_name  ("foo")                                              
                DW_AT_decl_file  ("/home/llvm-project/test.c")                   
                DW_AT_decl_line  (30)                                            
                DW_AT_declaration  (true)                                        
                DW_AT_external  (true)                                           
                                                                                 
0x0000004e:   DW_TAG_subprogram                                                  
                DW_AT_low_pc  (0x0000000b)                                       
                DW_AT_high_pc  (0x0000001a)                                      
                DW_AT_frame_base  (DW_OP_WASM_location 0x3 0x0, DW_OP_stack_value)
                DW_AT_GNU_all_call_sites  (true)                                 
                DW_AT_name  ("call_indirect")                                    
                DW_AT_decl_file  ("/home/llvm-project/test.c")                   
                DW_AT_decl_line  (3)                                             
                DW_AT_external  (true)                                           
                                                                                 
0x00000065:     DW_TAG_GNU_call_site                                             
                  DW_AT_low_pc  (0x00000019)                                     
                                                                                 
0x0000006a:     NULL                                                             
                                                                                 
0x0000006b:   NULL

It only seems to contain

0x00000065:     DW_TAG_GNU_call_site                                             
                  DW_AT_low_pc  (0x00000019)

for the indirect call. I'm not very familiar with dwarf info stuff myself, so please let me know if something is missing. Thanks.

djtodoro added inline comments.May 25 2021, 2:09 AM
llvm/test/DebugInfo/WebAssembly/call-site.ll
23

Actually, this is useless call_site debug information, since it does not have the DW_AT_GNU_call_site_target attribute (which holds the location of the indirect call).
In this case the call instruction is:

%6:i32 = CALL_INDIRECT 0, 0, %5:i32, %4:i32, %7:i32, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def dead $arguments, implicit $sp32, implicit $sp64, implicit-def $value_stack, implicit $value_stack, debug-location !11; test.c:40:11

And the callee op is %7:i32, which is not a physical register, so there is no way to generate MachineLocation for it. The WebAssembly indirect calls should be handled differently I guess.

For now, we can do:

const MachineOperand &CalleeOp = TII->getCalleeOperand(MI);
if (!CalleeOp.isGlobal() &&
    (!CalleeOp.isReg() || !Register::isPhysicalRegister(CalleeOp.getReg())))
   continue;
aheejin marked an inline comment as done.May 25 2021, 3:08 PM
aheejin updated this revision to Diff 347799.May 25 2021, 3:08 PM

Address comments

aheejin updated this revision to Diff 347800.May 25 2021, 3:09 PM

Comment fix

djtodoro accepted this revision.May 25 2021, 11:25 PM

Thanks!

This revision was landed with ongoing or failed builds.May 26 2021, 11:44 AM
This revision was automatically updated to reflect the committed changes.