Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: external asm parser doesn't check return code #4469

Closed
mattgodbolt opened this issue Dec 20, 2022 · 3 comments · Fixed by #4473
Closed

[BUG]: external asm parser doesn't check return code #4469

mattgodbolt opened this issue Dec 20, 2022 · 3 comments · Fixed by #4473
Labels

Comments

@mattgodbolt
Copy link
Member

Describe the bug

If the external parser fails (e.g. not found) then the error code goes unchecked and the JSON parser tries to parse the error output as JSON, leading to odd issues.

Steps to reproduce

  1. run local with externalparser=CEAsmParser and a externalparser.exe=/not/a/file
  2. compile binary
  3. note the error stacktrace is:
Internal Compiler Explorer error: SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
    at CEAsmParser.parseAsmExecResult (/home/matthew/dev/ce/compiler-explorer/lib/external-parsers/base.ts:76:59)
    at CEAsmParser.objdumpAndParseAssembly (/home/matthew/dev/ce/compiler-explorer/lib/external-parsers/base.ts:98:21)

Expected behavior

Error message like "external asm parser failed: ...OUTPUT HERE" (e.g. ./dump-and-parse.sh: line 4: /usr/local/bin/asm-parser: No such file or directory in my case)

Reproduction link

Not applicable

Screenshots

Not applicable

Operating System

No response

Browser version

No response

@partouf
Copy link
Contributor

partouf commented Dec 20, 2022

Should probably crash on startup. I don't think we should fiddle around with runtime error detection or something.

So add a file check here https://github.com/compiler-explorer/compiler-explorer/blob/main/lib/external-parsers/base.ts#L24 I think

mattgodbolt added a commit that referenced this issue Dec 20, 2022
Errors now look like:

```
Internal Compiler Explorer error: Error: Internal error running asm parser:
./dump-and-parse.sh: line 4: /usr/local/bin/asm-parser: No such file or directory

    at CEAsmParser.parseAsmExecResult (/home/matthew/dev/ce/compiler-explorer/lib/external-parsers/base.ts:77:19)
    at CEAsmParser.objdumpAndParseAssembly (/home/matthew/dev/ce/compiler-explorer/lib/external-parsers/base.ts:101:21)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async DefaultCompiler.objdump (/home/matthew/dev/ce/compiler-explorer/lib/base-compiler.ts:414:31)
    at async Promise.all (index 0)
    at async DefaultCompiler.checkOutputFileAndDoPostProcess (/home/matthew/dev/ce/compiler-explorer/lib/base-compiler.ts:1356:16)
    at async /home/matthew/dev/ce/compiler-explorer/lib/base-compiler.ts:2236:41
    at async run (/home/matthew/dev/ce/compiler-explorer/node_modules/p-queue/dist/index.js:163:29)
Compiler returned: -1
```

which at least gives context on why the parsing failed.
@mattgodbolt mattgodbolt mentioned this issue Dec 20, 2022
@mattgodbolt
Copy link
Member Author

I think it's still worth checking. If the process quits for other reasons (SEGV) etc, it's useful to know rather than getting a cryptic JSON parse message.

@mattgodbolt
Copy link
Member Author

I'll add the check too for earlier failing.

mattgodbolt added a commit that referenced this issue Jan 24, 2023
* Fix #4469

Errors now look like:

```
Internal Compiler Explorer error: Error: Internal error running asm parser:
./dump-and-parse.sh: line 4: /usr/local/bin/asm-parser: No such file or directory

    at CEAsmParser.parseAsmExecResult (/home/matthew/dev/ce/compiler-explorer/lib/external-parsers/base.ts:77:19)
    at CEAsmParser.objdumpAndParseAssembly (/home/matthew/dev/ce/compiler-explorer/lib/external-parsers/base.ts:101:21)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async DefaultCompiler.objdump (/home/matthew/dev/ce/compiler-explorer/lib/base-compiler.ts:414:31)
    at async Promise.all (index 0)
    at async DefaultCompiler.checkOutputFileAndDoPostProcess (/home/matthew/dev/ce/compiler-explorer/lib/base-compiler.ts:1356:16)
    at async /home/matthew/dev/ce/compiler-explorer/lib/base-compiler.ts:2236:41
    at async run (/home/matthew/dev/ce/compiler-explorer/node_modules/p-queue/dist/index.js:163:29)
Compiler returned: -1
```

which at least gives context on why the parsing failed. And per discussion with @partouf we'll blow up at construction time if the file isn't there, which means the above is more to cover "what if the tool dies".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants