This is an archive of the discontinued LLVM Phabricator instance.

[Utility] Placate another GCC warning
ClosedPublic

Authored by davide on Apr 17 2017, 2:08 PM.

Details

Summary

For reference/discussion.
GCC complains about signed-vs-unsigned comparison. I'm actually surprised that m_registers_count is a signed integer, as I can hardly imagine a negative register count. I'm under the impression that we could change m_register_count fields to be unsigned, but that would be a larger change. Thoughts?

Diff Detail

Repository
rL LLVM

Event Timeline

davide created this revision.Apr 17 2017, 2:08 PM
labath added a subscriber: labath.Apr 18 2017, 3:52 AM

I am confused. m_registers_count is declared as uint8_t at RegisterContextPOSIX_mips64.h:67...

I'm getting more convinced the warning emitted by GCC is bogus

/home/davide/work/llvm-lldb/tools/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_mips64.cpp:58:26: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   assert(m_num_registers == m_registers_count[gpr_registers_count] +
          ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                             m_registers_count[fpr_registers_count] +
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                             m_registers_count[msa_registers_count]);
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

And I was wrong. @nlewycky explained on IRC:

14:23 < nlewycky> gcc is correct, in 'char + char' each char gets promoted to 'int' first then summed, then you've got an unsigned int == int comparison
14:23 < nlewycky> uint8_t and unsigned char are also promoted to int (not unsigned int) before the addition
sas accepted this revision.Apr 19 2017, 5:40 PM
sas added a subscriber: sas.
sas added inline comments.
source/Plugins/Process/Utility/RegisterContextPOSIX_mips64.cpp
59 ↗(On Diff #95485)

If you're gonna go with casting, I think using the same type as m_num_registers makes more sense.

assert(m_num_registers == static_cast<uint32_t>(...));
This revision is now accepted and ready to land.Apr 19 2017, 5:40 PM

Thanks for digging into this, I've learned something new today. Fixing this with a cast seems reasonable then.

Thanks for digging into this, I've learned something new today. Fixing this with a cast seems reasonable then.

Me too, apparently C++ integer promotion is less obvious than I thought ;)

This revision was automatically updated to reflect the committed changes.