This is an archive of the discontinued LLVM Phabricator instance.

EmulationStateARM: Don't have >= comparisons against dwarf_r0
AbandonedPublic

Authored by zturner on Jul 22 2014, 1:44 PM.

Details

Reviewers
majnemer
tfiala
Summary

dwarf_r0 is defined to be zero in ARM_DWARF_Registers.h, this makes
unsigned comparisons against it pointless and provokes warnings on GCC.

Diff Detail

Event Timeline

majnemer updated this revision to Diff 11781.Jul 22 2014, 1:44 PM
majnemer retitled this revision from to EmulationStateARM: Don't have >= comparisons against dwarf_r0.
majnemer updated this object.
majnemer added reviewers: zturner, tfiala.
majnemer added a subscriber: Unknown Object (MLST).
zturner edited edge metadata.Jul 30 2014, 1:33 PM

For trivial warning fixes you can probably just submit.

tfiala edited edge metadata.Jul 30 2014, 1:39 PM

I dunno - this is encoding some logic that makes sure the reg number is
between r0 and cpsr. The fact that r0 happens to be zero is convenient in
this case, but taking that out is taking out some documentation of what the
code is really doing.

In general I'm not a fan of removing semantic information that is
meaningful to humans even if the compiler can optimize it to a simpler
expression. If this code were ever copied for another architecture where
the equivalent of r0 was a different number, or if the reg_num was turned
to a typedef and that changed to, say, an int, we've not only lost semantic
info but also real logic.

That's my 2 cents.

-Todd

Could probably address that concern with a comment, but the alternatives
(disabling the warning or doing a cast to a signed type) both seem strictly
worse than removing the code, and I'm a strong proponent of warning-free
code across all platforms.

Or you can disable warnings for that type, right? "Don't warn about
comparsions to 0 for type {enum type}, because the whole reason I'm using
enums is so I don't have to worry about the value of the enum."

Don't let the compiler dictate writing less safe code.

That's like that lame switch coverage warning that tells you "you have a
(supposedly superfluous) default when all enum values are covered by the
switch". And if the compiler can guarantee that something else in the
program doesn't inadvertently permute the switch variable to a value
outside the values of the enum, then great. If not, that's a dangerous
warning. I can't tell you the number of times I was glad I had a default
with some kind of sanity check on a game server to tip off a gnarly memory
overwrite condition. Yet the easy thing to do here would be to remove the
default, and miss catching the case of a bogus value.

/rant off.

In this case, a comment is likely sufficient. Another way to work around
this is to use a range-check method where the lower and upper bounds are
passed in, which would likely preserve both the intent and eliminate the
warning in one foul swoop :-)

zturner commandeered this revision.Oct 30 2014, 12:56 PM
zturner abandoned this revision.
zturner edited reviewers, added: majnemer; removed: zturner.