Crash with asm volatile

[Sahnvour]

I was toying with inline assembly syntax to try to reproduce the DoNotOptimize function from google/benchmark https://github.com/google/benchmark/blob/master/include/benchmark/benchmark.h#L309-L326.

I wrote this https://godbolt.org/z/eExl8y by trial and error since there's no inline asm documentation yet.
Ouput operands cannot be empty do I added a dummy variable to satisfy the language.

Now even insignificant modifications to this code can cause a compiler crash:

• try adding a blank line before the asm volatile
• try modifying the xxx identifier
• try changing num to be a value instead of a pointer
• try using a directly inside the asm instead of its address

These only happen in release modes.
I'm not sure what belongs to bugs and to invalid input, but I guess the compiler should at least not crash.

[Sahnvour]

Did a bit more testing:

• crash happens inside llvm code when optimizing with assembly/binary output (works fine on --emit llvm-ir)
• crash seems random
• rarely just outputs a message like error: couldn't allocate output register for constraint 'a' instead of crashing, sometimes works fine

Callstack if that's of any help:

llvm::FunctionLoweringInfo::InitializeRegForValue(class llvm::Value const *)
llvm::SelectionDAGBuilder::getValueImpl(class llvm::Value const *)
llvm::SelectionDAGBuilder::getValue(class llvm::Value const *)
llvm::SelectionDAGBuilder::visitInlineAsm(class llvm::ImmutableCallSite)
llvm::SelectionDAGBuilder::visit(unsigned int,class llvm::User const &)
llvm::SelectionDAGBuilder::visit(class llvm::Instruction const &)
llvm::SelectionDAGISel::SelectBasicBlock(class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::Instruction,0,0,void>,0,1>,class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::Instruction,0,0,void>,0,1>,bool &)
llvm::SelectionDAGISel::SelectAllBasicBlocks(class llvm::Function const &)
llvm::SelectionDAGISel::runOnMachineFunction(class llvm::MachineFunction &)
llvm::SDNode::hasPredecessorHelper(class llvm::SDNode const *,class llvm::SmallPtrSetImpl<class llvm::SDNode const *> &,class llvm::SmallVectorImpl<class llvm::SDNode const *> &,unsigned int,bool)
llvm::MachineFunctionPass::runOnFunction(class llvm::Function &)
llvm::FPPassManager::runOnFunction(class llvm::Function &)
llvm::FPPassManager::runOnModule(class llvm::Module &)
llvm::FPPassManager::runOnModule(class llvm::Module &)
llvm::legacy::PassManager::run(class llvm::Module &)
ZigLLVMTargetMachineEmitToFile(LLVMOpaqueTargetMachine * targ_machine_ref, LLVMOpaqueModule * module_ref, const char * filename, ZigLLVM_EmitOutputType output_type, char * * error_message, bool is_debug, bool is_small, bool time_report)
zig_llvm_emit_output(CodeGen * g)
codegen_build_and_link(CodeGen * g)
main(int argc, char * * argv)

So I guess the modifications I listed in my previous comment are irrelevant, they just help avoiding CE's cache and trigger a new compilation, that may or may not crash.

[andrewrk]

Thanks for doing that research. Looks like we may need to report this upstream. Are you using an assertion-enabled LLVM?

[Sahnvour]

I only have the release build you supplied right now but I plan to do a debug one eventually.
Can we be sure the roots of the problem aren't on Zig side (passing incoherent IR or whatever) ?

[andrewrk]

That's one reason I want to run the test with assertions on - it is likely to catch a problem caused by Zig using the API incorrectly.

[Sahnvour]

Update (I'm learning as I go, so please correct me if needed):

In the example I linked from google/benchmark, the comma in the constraints is used to denote alternative constraints, ie. the compiler is free to chose r or m. This notation is inherited from GCC, the most accurate source I found on this is (very old) https://gcc.gnu.org/ml/gcc/1999-10n/msg00488.html (also https://gcc.gnu.org/ml/gcc/2015-10/msg00249.html ).
However, Zig concatenates constraints and passes them directly as a comma-separated string to LLVM API. So according to http://llvm.org/docs/LangRef.html#constraint-codes LLVM will parse more constraints than there actually are inputs, and it does not end well.
https://groups.google.com/forum/#!topic/llvm-dev/0CR27OFlTAo suggests that Clang handles this in the frontend.

So if I understand correclty, replacing commas in constraints by | should be enough to work this out if we want to be as compatible with GCC/Clang inline assembly as possible.

However, in this very case, I don't understand why the source of google/benchmark uses r,m instead of rm since there is no other intput/output constraint that would also need two alternative constraint sets.

[Sahnvour]

Found #215 (and TODOs) while investigating other crashes related to asm, for example when using comptime_ints as inputs. To what extent do you think these should be addressed before the big overhaul that is planned ?

[andrewrk]

I think we should definitely prioritize fixing the crashes. These are typically easy, small fixes that make a big difference in user experience. Worst case scenario they can emit compile errors that say "TODO" and link to the relevant issue.

I'll have to take a look at those docs you linked. At this point you're probably more knowledgeable about this topic than me; it's been a while since I visited the inline assembly code in Zig.

I know that it's one of the first things I implemented because I needed it for syscalls and it badly needs revisiting.

[Sahnvour]

OK, hopefully I will have time to fix some in the next few days then.