This is an archive of the discontinued LLVM Phabricator instance.

Check existence of each required component during construction of LLVMCDisassembler.
ClosedPublic

Authored by tatyana-krasnukha on Dec 26 2017, 8:11 AM.

Details

Summary

Actually, fix two issues:

  1. remove repeat creation of reg_info, use m_reg_info_ap for createMCAsmInfo instead;
  2. remove possibility to dereference nullptr during createMCAsmInfo invocation, that could lead to undefined behavior.

Placed checking of a component right after its creation to simplify the code and avoid same issues later.

Event Timeline

labath requested changes to this revision.Jan 9 2018, 4:31 AM
labath added a subscriber: labath.

I think this makes the code hard to read. A much better approach would be to use Expected<T> + factory function pattern. So you would have something like:

Expected<unique_ptr<LLVMCDisassembler>> LLVMCDisassembler::Create(...) {
  do_first_thing_that_can_fail(...);
  if (first_thing_failed)
    return make_string_error("First thing failed");
  do_second_thing_that_can_fail();
  ...
  return make_unique<LLVMCDisassembler>(...); // can't fail
}

I think this is much more readable than the lambda approach, and it also allows you to get rid of the IsValid() function, as we only construct valid objects.

This revision now requires changes to proceed.Jan 9 2018, 4:31 AM

Thank you, Pavel.
Would you mind if I move LLVMCDisassembler declaration in .cpp also? It looks like perfect candidate for pimpl.

And... doesn't DisassemblerLLVMC::LLVMCDisassembler confuse anyone but me?)

labath added a comment.Jan 9 2018, 6:24 AM

Thank you, Pavel.
Would you mind if I move LLVMCDisassembler declaration in .cpp also? It looks like perfect candidate for pimpl.

Yes, that sounds like a good idea.

And... doesn't DisassemblerLLVMC::LLVMCDisassembler confuse anyone but me?)

Well... I haven't looked at this class in the past, but yes... it looks confusing. I wouldn't mind a name change.

Jason, any thoughts on this?

Added function Create that creates an instance of LLVMCDisassembler only if pass all constraints.
Moved LLVMCDisassembler declaration to .cpp file, renamed to MCDisasmToolset (is this name ok?).
Added const qualifier to some functions of the class.
I also had the courage to remove ‘_ap’ suffixes from unique pointers.

clayborg requested changes to this revision.Jan 9 2018, 11:08 AM
clayborg added inline comments.
source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
77–83

add "_up" suffix to all std::unique_ptr member variables.

989–1003

Do we need this? We are placing the MCDisasmToolset into std::unique_ptr<> member variables so this shouldn't be needed by anyone as the std::unique_ptr already has a move operator

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h
102

Maybe "MCDisasmInstance", "MCDisassembler" or just "Disassembler"?

103

add "_up" suffix to anything that is a std::unique_ptr

104

ditto

This revision now requires changes to proceed.Jan 9 2018, 11:08 AM

I like the overall direction this patch is taking, just a few fixes.

labath accepted this revision.Jan 10 2018, 2:37 AM

Looks good to me. Just make sure to respond to all of Greg's comments as well.

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
989–1003

The std::move is necessary in this context. The rvalue references can be dropped theoretically -- that will enforce that the constructor takes ownership of the passed-in arguments (as they will get implicitly deleted otherwise) -- right now the constructor can just do nothing and the caller will keep owning these objects.
However, this distinction seldom matters.

1002

this should be m_disasm and m_instr_printer (and make sure to test this with assertions enabled).

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
1002

Thanks, I was not aware of this CMake option. Of course assertion failed.

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h
102

MCDisassembler already exists in llvm namespace (it is one of members of this class). Disassembler is base class of DisassemblerLLVMC. l thought about MCDisasmImpl also. Probably MCDisasmInstance is even better.

Added "_up" suffix to each unique_ptr, renamed MCDisasmToolset to MCDisasmInstance.

tatyana-krasnukha marked 5 inline comments as done.Jan 10 2018, 6:18 AM

There is the function GetDisasmToUse in InstructionLLVMC class that can return nullptr. But this case is not handled in any usage. I suppose that caller functions cannot be invoked if !DisassemblerLLVMC::IsValid(). But it still looks dangerous for me. May be GetDisasmToUse should assert if neither m_disasm_up nor m_alternate_disasm_up exists? And may return a reference then.

clayborg accepted this revision.Jan 10 2018, 1:34 PM

Fix comment spacing as mentioned in inline comments and this is good to go.

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h
98–104

Fix spacing on these comments since they are new.

This revision is now accepted and ready to land.Jan 10 2018, 1:34 PM

Fixed comment spacing. Changed the comment slightly (words “I added this class to…” sound now like I did this, so, replaced it with passive voice).