This is an archive of the discontinued LLVM Phabricator instance.

MCRegisterInfo: Merge getLLVMRegNum and getLLVMRegNumFromEH
ClosedPublic

Authored by labath on Sep 4 2019, 5:06 AM.

Details

Summary

The functions different in two ways:

  • getLLVMRegNum could return both "eh" and "other" dwarf register numbers, while getLLVMRegNumFromEH only returned the "eh" number.
  • getLLVMRegNum asserted if the register was not found, while the second function returned -1.

The second distinction was pretty important, but it was very hard to
infer that from the function name. Aditionally, for the use case of
dumping dwarf expressions, we needed a function which can work with both
kinds of number, but does not assert.

This patch solves both of these issues by merging the two functions into
one, returning an Optional<unsigned> value. While the same thing could
be achieved by adding an "IsEH" argument to the (renamed)
getLLVMRegNumFromEH function, it seemed better to avoid the confusion of
two functions and put the choice of asserting into the hands of the
caller -- if he checks the Optional value, he can safely process
"untrusted" input, and if he blindly dereferences the Optional, he gets
the assertion.

I've updated all call sites to the new API, choosing between the two
options according to the function they were calling originally, except
that I've updated the usage in DWARFExpression.cpp to use the "safe"
method instead, and added a test case which would have previously
triggered an assertion failure when processing (incorrect?) dwarf
expressions.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Sep 4 2019, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2019, 5:06 AM
JDevlieghere accepted this revision.Sep 4 2019, 5:59 PM

This makes sense to me.

This revision is now accepted and ready to land.Sep 4 2019, 5:59 PM
This revision was automatically updated to reflect the committed changes.