[GISel]: Fix undefined behavior while accessing DefaultAction map

[aditya_nandakumar]

Summary

We end up dereferencing the end iterator here when the Aspect doesn't exist in the DefaultAction map.
Check for end iterator.

[dsanders]

include/llvm/CodeGen/GlobalISel/LegalizerInfo.h

159

I'm not sure about this bit. Are the callers expecting to deal with an invalid LLT?

[aditya_nandakumar]

Updated the Patch to make the helper function return Optional<LLT> which the caller checks for validity.

[aditya_nandakumar]

Updated API to return Optional<LLT>

[qcolombet]

Hi Aditya,

LGTM with the optional update.
@dsanders Daniel, I believe it fixes your concern, right?

Cheers,
-Quentin

[dsanders]

Yep, LGTM.

One nit: We could avoid introducing the Optional<> by testing for LLT.isValid() instead of testing for whether the Optional has a value

[aditya_nandakumar]

Sure - LLT.isValid() also sounds good - I'll go ahead and do it that way.

[aditya_nandakumar]

committed in r302963

include/llvm/CodeGen/GlobalISel/LegalizerInfo.h

public:
   /// \returns a tuple consisting of the LegalizeAction that should be
   /// performed, the type-index it should be performed on and the destination
   /// type.
   std::tuple<LegalizeAction, unsigned, LLT>
   getAction(const MachineInstr &MI, const MachineRegisterInfo &MRI) const;
 
   /// Iterate the given function (typically something like doubling the width)
   /// on Ty until we find a legal type for this operation.
-  LLT findLegalType(const InstrAspect &Aspect,
+  Optional<LLT> findLegalType(const InstrAspect &Aspect,
                     function_ref<LLT(LLT)> NextType) const {
     LegalizeAction Action;
     const TypeMap &Map = Actions[Aspect.Opcode - FirstOp][Aspect.Idx];
     LLT Ty = Aspect.Type;
     do {
       Ty = NextType(Ty);
       auto ActionIt = Map.find(Ty);
-      if (ActionIt == Map.end())
-        Action = DefaultActions.find(Aspect.Opcode)->second;
+      if (ActionIt == Map.end()) {
+        auto DefaultIt = DefaultActions.find(Aspect.Opcode);
+        if (DefaultIt == DefaultActions.end())
+          return None;
+        Action = DefaultIt->second;
+      }
       else
         Action = ActionIt->second;
     } while(Action != Legal);
     return Ty;
   }
 
   /// Find what type it's actually OK to perform the given operation on, given
   /// the general approach we've decided to take.
-  LLT findLegalType(const InstrAspect &Aspect, LegalizeAction Action) const;
+  Optional<LLT> findLegalType(const InstrAspect &Aspect, LegalizeAction Action) const;
 
   std::pair<LegalizeAction, LLT> findLegalAction(const InstrAspect &Aspect,
                                                  LegalizeAction Action) const {
-    return std::make_pair(Action, findLegalType(Aspect, Action));
+    auto LegalType = findLegalType(Aspect, Action);
+    if (!LegalType)
+      return std::make_pair(LegalizeAction::Unsupported, LLT());
+    return std::make_pair(Action, *LegalType);
   }
 
   /// Find the specified \p Aspect in the primary (explicitly set) Actions
   /// table. Returns either the action the target requested or NotFound if there
   /// was no setAction call.
   LegalizeAction findInActions(const InstrAspect &Aspect) const {
     if (Aspect.Opcode < FirstOp || Aspect.Opcode > LastOp)
       return NotFound;

lib/CodeGen/GlobalISel/LegalizerInfo.cpp

LegalizerInfo::getAction(const MachineInstr &MI,
   return std::make_tuple(Legal, 0, LLT{});
 }
 
 bool LegalizerInfo::isLegal(const MachineInstr &MI,
                             const MachineRegisterInfo &MRI) const {
   return std::get<0>(getAction(MI, MRI)) == Legal;
 }
 
-LLT LegalizerInfo::findLegalType(const InstrAspect &Aspect,
+Optional<LLT> LegalizerInfo::findLegalType(const InstrAspect &Aspect,
                                  LegalizeAction Action) const {
   switch(Action) {
   default:
     llvm_unreachable("Cannot find legal type");
   case Legal:
   case Lower:
   case Libcall:
   case Custom: