carbon-language / carbon-lang

Carbon Language's main repository: documents, design, implementation, and related tools. (NOTE: Carbon Language is experimental; see README)
http://docs.carbon-lang.dev/
Other
32.24k stars 1.48k forks source link

Use verbose formatting of instructions on crash messages. #4125

Closed jonmeow closed 2 months ago

jonmeow commented 2 months ago

Changes crash messages to start printing verbose forms of instructions, rather than just the ID. Fixes some indentation issues with stacks. Also switches unexpected inst formatting, because now there are lots, and it'd be helpful to know where they are.

This uses a pimpl pattern for Formatter due to the number of member functions on Formatter. Maybe we should refactor that, but this didn't feel like a good place to do so.

Note, I have two concerns about this change... to note them here, to make sure others are considering them when evaluating the implementation:

  1. Some instructions are very verbose to print, as evidenced by the fn_decl printing (which includes function params) or scope printing (which includes scope members).
    • I'm not sure whether there's a way to simply reduce this, as it seems essential to the requested printing of instructions.
    • Long-term, we may at least want to limit the number of lines printed here. However, I've already spent a fair amount of time here and I think it's in a good state to evaluate.
  2. Increased complexity in the crash handler may result in crash messages failing to generate.
    • For example, a crash in Formatter (and its deps, such as InstNamer or location handling) prevents a stack from being printed. I'm pretty sure I've written crashes in Formatter before.

Here's an example crash snippet (generated by adding a crash inside return handling) before:

2.  NodeStack:
    0.  FunctionDefinitionStart -> function2
    1.  ReturnStatementStart -> no value
    2.  IntLiteral -> inst+26
inst_block_stack_:
    0.  block<invalid>  {inst+0, inst+1, inst+2, inst+23}
    1.  block9  {inst+26}
param_and_arg_refs_stack:
args_type_info_stack_:

And after:

2.  Check::Context
          NodeStack:
            0. FunctionDefinitionStart: function2
            1. ReturnStatementStart: no value
            2. IntLiteral:
              unexpected.inst+26.loc12_10: i32 = int_literal 0 [template = constants.%.2]
          inst_block_stack_:
            0. block<invalid> {
                package: <namespace> = namespace [template] {
                  .Core = unexpected.inst+2
                  .F = unexpected.inst+23.loc11_22
                }
                unexpected.inst+1 = import Core
                unexpected.inst+2: <namespace> = namespace unexpected.inst+1, [template] {}
                unexpected.inst+23.loc11_22: %F.type = fn_decl @F [template = constants.%F] {
                  unexpected.inst+9.loc11_9: init type = call constants.%Bool() [template = bool]
                  unexpected.inst+10.loc11_9: type = value_of_initializer unexpected.inst+9.loc11_9 [template = bool]
                  unexpected.inst+11.loc11_9: type = converted unexpected.inst+9.loc11_9, unexpected.inst+10.loc11_9 [template = bool]
                  unexpected.inst+12.loc11_6: bool = param b
                  @F.%b: bool = bind_name b, unexpected.inst+12.loc11_6
                  unexpected.inst+19.loc11_18: init type = call constants.%Int32() [template = i32]
                  unexpected.inst+20.loc11_18: type = value_of_initializer unexpected.inst+19.loc11_18 [template = i32]
                  unexpected.inst+21.loc11_18: type = converted unexpected.inst+19.loc11_18, unexpected.inst+20.loc11_18 [template = i32]
                  @F.%return: ref i32 = var <return slot>
                }
              }
            1. block9 {
                unexpected.inst+26.loc12_10: i32 = int_literal 0 [template = constants.%.2]
              }
          param_and_arg_refs_stack:
          args_type_info_stack_:
jonmeow commented 2 months ago

Note, per side-discussion about the risks, we'll try this out. People will be asked to comment to #toolchain if they use the more detailed info. If nobody's using it, we may back it out (at least in part)