This is an archive of the discontinued LLVM Phabricator instance.

Add assertion to detect invalid registers in the PowerPC MC instruction lowering
ClosedPublic

Authored by sfantao on Mar 17 2015, 10:56 AM.

Details

Reviewers
hfinkel
Summary

We have observed that noreg was being generated due to a bug in FastIsel and was not being detected during emission. It happens that in the Asm emission there is an assertion that detects this in getRegisterName() from the tbl-generated file PPCGenAsmWriter.inc. However, when emitting an Obj file, invalid registers can be emitted given that no check are made in getBinaryCodeFromInstr() from PPCGenMCCodeEmitter.inc. In order to cover all cases I added an assertion for reg operands in LowerPPCMachineInstrToMCInst. The intent is to take advantage of the assertion in getRegisterName more than the value returned by it. If one wants to detect noreg only, we could replace this by a comparison against zero, but this seemed more robust to me.

Thanks,
Samuel

Diff Detail

Event Timeline

sfantao updated this revision to Diff 22103.Mar 17 2015, 10:56 AM
sfantao retitled this revision from to Add assertion to detect invalid registers in the PowerPC MC instruction lowering.
sfantao updated this object.
sfantao edited the test plan for this revision. (Show Details)
sfantao added a reviewer: hfinkel.
sfantao added a subscriber: Unknown Object (MLST).
hfinkel edited edge metadata.Mar 17 2015, 11:05 AM

Thanks for looking into this. I'd prefer to use the enum fields directly, however, instead of calling getRegisterName. We should just be able to write this:

assert(MO.getReg() > PPC::NoRegister && MO.getReg() < PPC::NUM_TARGET_REGS &&
            "Invalid register for this target!");
sfantao updated this revision to Diff 22108.Mar 17 2015, 11:24 AM
sfantao edited edge metadata.

I agree, I didn't notice we had PPC::NUM_TARGET_REGS.

Thanks!
Samuel

hfinkel accepted this revision.Mar 17 2015, 11:31 AM
hfinkel edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Mar 17 2015, 11:31 AM
sfantao closed this revision.Mar 17 2015, 12:35 PM

Committed in r232525.

Thanks!
Samuel