3

There is an existing function named "Compare," which is

int compare(void* A, void* B) { return (int)A - (int)B; }

I am aware that this is an atrocious practice, but I did not write that piece of code and it is being used in many places already. But this code was generating a compilation error under 64-bit, since void* is no longer 32-bit, so I fixed the code to the following. 

int compare(void* A, void* B) { return (long long)A - (long long)B; }

What is the likelihood of this function returning an incorrect result under current 64-bit Linux architecture? i.e, what is the likelihood of two virtual addresses being apart for more than 0x7FFFFFFF?

xorxorxor
  • 65
  • 5
  • 4
    I'd recommend using `uintptr_t` instead of `long long`. – Bill Lynch Oct 28 '11 at 17:58
  • 1
    Clearly, the likelihood is 1/2. You will get a reliable result on both 32-bit and 64-bit systems if you take the difference between the two pointers directly, rather than trying to shoehorn them into some int or other. The difference of (void*) A - (void*) B yields a difference of type ptrdiff_t which you can handle easily. – Pete Wilson Oct 28 '11 at 18:06
  • How is this function used? If it's used as a comparison function for `qsort()`, it's not going to work properly. If not, converting the pointers to *any* integer type makes no sense; just use direct pointer subtraction (which yields a `ptrdiff_t`). – Keith Thompson Oct 28 '11 at 18:09
  • @KeithThompson: You can't subtract pointers to an incomplete type. – Ben Voigt Oct 28 '11 at 18:17
  • @BenVoigt: D'oh, you're right. `(char*)A - (char*)B` will work, though. – Keith Thompson Oct 28 '11 at 18:18
  • "Atrocious practice" - I disagree strongly. Sometimes it's necessary with "frowned upon"-constructs. It's like religion: you have the orthodox and those just trying to get on with their day-to-day lives. The orthodox may give any number of reasons for you to not do something and, if they offer an alternative, it will likely be more or less in conflict with you trying to get on with your life (work in this case) in a practical manner. – Olof Forshell Nov 01 '11 at 09:52

7 Answers7

7

I think you want

int compare(void* A, void* B) { return (A > B) - (A < B); }
Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • Unlike difference based code this doesn't suffer from int-overflow issues. Of course it's still undefined behavior, but that's inherent in the OPs requirements. – CodesInChaos Oct 29 '11 at 19:27
3

On my linux machine, here's an example.

I have a copy of tcsh running. It has a process id of 9732. We can look at the memory maps of it by examining /proc/<pid>/maps.

From the table below, we can see that heap data is stored around 0x01e30000, while stack data is stored around 0x7fffca3e6000. So in a simple case, if you compare a malloc allocated pointer with a stack pointer, you'll see a significant difference in pointers.

[1:02pm][wlynch@charcoal Harrow] cat /proc/9732/maps
00400000-0045a000 r-xp 00000000 09:00 44826689                           /bin/tcsh
0065a000-0065e000 rw-p 0005a000 09:00 44826689                           /bin/tcsh
0065e000-00674000 rw-p 00000000 00:00 0 
0085d000-0085f000 rw-p 0005d000 09:00 44826689                           /bin/tcsh
01e30000-01f78000 rw-p 00000000 00:00 0                                  [heap]
38a3c00000-38a3c1e000 r-xp 00000000 09:00 16253177                       /lib64/ld-2.12.so
38a3e1e000-38a3e1f000 r--p 0001e000 09:00 16253177                       /lib64/ld-2.12.so
38a3e1f000-38a3e20000 rw-p 0001f000 09:00 16253177                       /lib64/ld-2.12.so
38a3e20000-38a3e21000 rw-p 00000000 00:00 0 
38a4400000-38a4575000 r-xp 00000000 09:00 16253179                       /lib64/libc-2.12.so
38a4575000-38a4775000 ---p 00175000 09:00 16253179                       /lib64/libc-2.12.so
38a4775000-38a4779000 r--p 00175000 09:00 16253179                       /lib64/libc-2.12.so
38a4779000-38a477a000 rw-p 00179000 09:00 16253179                       /lib64/libc-2.12.so
38a477a000-38a477f000 rw-p 00000000 00:00 0 
38a4800000-38a4802000 r-xp 00000000 09:00 16253186                       /lib64/libdl-2.12.so
38a4802000-38a4a02000 ---p 00002000 09:00 16253186                       /lib64/libdl-2.12.so
38a4a02000-38a4a03000 r--p 00002000 09:00 16253186                       /lib64/libdl-2.12.so
38a4a03000-38a4a04000 rw-p 00003000 09:00 16253186                       /lib64/libdl-2.12.so
38af000000-38af01d000 r-xp 00000000 09:00 16253156                       /lib64/libtinfo.so.5.7
38af01d000-38af21d000 ---p 0001d000 09:00 16253156                       /lib64/libtinfo.so.5.7
38af21d000-38af221000 rw-p 0001d000 09:00 16253156                       /lib64/libtinfo.so.5.7
38b0c00000-38b0c58000 r-xp 00000000 09:00 16253191                       /lib64/libfreebl3.so
38b0c58000-38b0e57000 ---p 00058000 09:00 16253191                       /lib64/libfreebl3.so
38b0e57000-38b0e59000 rw-p 00057000 09:00 16253191                       /lib64/libfreebl3.so
38b0e59000-38b0e5d000 rw-p 00000000 00:00 0 
38b1000000-38b1007000 r-xp 00000000 09:00 16253192                       /lib64/libcrypt-2.12.so
38b1007000-38b1207000 ---p 00007000 09:00 16253192                       /lib64/libcrypt-2.12.so
38b1207000-38b1208000 r--p 00007000 09:00 16253192                       /lib64/libcrypt-2.12.so
38b1208000-38b1209000 rw-p 00008000 09:00 16253192                       /lib64/libcrypt-2.12.so
38b1209000-38b1237000 rw-p 00000000 00:00 0 
7f03aa9a0000-7f03aa9ac000 r-xp 00000000 09:00 16252957                   /lib64/libnss_files-2.12.so
7f03aa9ac000-7f03aabab000 ---p 0000c000 09:00 16252957                   /lib64/libnss_files-2.12.so
7f03aabab000-7f03aabac000 r--p 0000b000 09:00 16252957                   /lib64/libnss_files-2.12.so
7f03aabac000-7f03aabad000 rw-p 0000c000 09:00 16252957                   /lib64/libnss_files-2.12.so
7f03aabbc000-7f03aabc3000 r--s 00000000 09:00 5769665                    /usr/lib64/gconv/gconv-modules.cache
7f03aabc3000-7f03b0a54000 r--p 00000000 09:00 5506757                    /usr/lib/locale/locale-archive
7f03b0a54000-7f03b0a58000 rw-p 00000000 00:00 0 
7f03b0a5b000-7f03b0a67000 r--p 00000000 09:00 5510943                    /usr/share/locale/en/LC_MESSAGES/tcsh
7f03b0a67000-7f03b0a68000 rw-p 00000000 00:00 0 
7fffca3e6000-7fffca3fb000 rw-p 00000000 00:00 0                          [stack]
7fffca3ff000-7fffca400000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Bill Lynch
  • 80,138
  • 16
  • 128
  • 173
  • 1
    The behavior of subtracting or comparing pointers to distinct objects (e.g., one to a stack object and one to a heap object) is undefined. You can probably get away with it on most systems, though. – Keith Thompson Oct 28 '11 at 18:21
  • OTOH, comparing pointers which aren't part of the same composite object causes undefined behavior anyway. – Ben Voigt Oct 28 '11 at 18:21
2

This looks like a sort comparison function, so all that matters is the sign of the result.

int compare(void *A, void* B)
{
  if (A < B) return -1;
  if (A > B) return 1;
  return 0;
 }
Raymond Chen
  • 44,448
  • 11
  • 96
  • 135
  • It certainly looks like it -- but if it just compare addresses, then it doesn't make sense as a comparison function for `qsort()`. `qsort()` sorts an array, whose elements are *already* sorted by address. And even if that weren't the case, swapping elements would change their addresses, making the comparison results inconsistent. – Keith Thompson Oct 28 '11 at 18:06
  • @KeithThompson: Maybe you have a collection of pointers which you want sorted (the pointer is the element, not the address of the element). Or you are trying to do binary search in a sorted table of pointers. – Ben Voigt Oct 28 '11 at 18:19
  • @BenVoigt: Ok, but the arguments to the `qsort` compare function are the addresses of the elements, not their values. – Keith Thompson Oct 28 '11 at 18:23
  • @KeithThompson: You're the only one saying anything about `qsort`. – Ben Voigt Oct 28 '11 at 18:24
0

You shouldn't be returning an int, but rather a uintptr_t. Make sure variables set to this function are uintptr_t too.

uintptr_t compare(void* A, void* B) { return (uintptr_t)A - (uintptr_t)B; }

If you can't do this for whatever reason, then you should be checking that the values are within range and throw an error if not. The chance is probably very small, but that doesn't mean it's an exceptional case.

Pubby
  • 51,882
  • 13
  • 139
  • 180
  • `size_t` isn't guaranteed to be able to hold a pointer. He should indeed return a `size_t`, but the cast should be to something else. – Paul Manta Oct 28 '11 at 18:02
  • @Pubby8: a [uintptr_t](http://linux.die.net/man/3/uintptr_t) is defined to be able to hold a pointer. – Bill Lynch Oct 28 '11 at 18:04
  • 1
    Don't convert to `intptr_t`. Just compare the pointers themselves. – Keith Thompson Oct 28 '11 at 18:07
  • @KeithThompson: No need to convert if you're merely comparing. Only for pointer arithmetic (which needs to scale by `sizeof (element_type)`). – Ben Voigt Oct 28 '11 at 18:23
  • @BenVoigt: You're right. Apparently I haven't had enough coffee. – Keith Thompson Oct 28 '11 at 18:24
  • 3
    The point of three-way comparison functions is to return negative, zero or positive depending on the ordering of the arguments. If you return an unsigned type such as uintptr_t you're never going to return a negative value. So, you want intptr_t. – Richard Kettlewell Oct 29 '11 at 11:02
  • Am I missing something, or this this function completely nonsensical? The result is always >=0, and as such violates the requirement of `sign(Compare(a,b))=-sign(Compare(b,a))`. – CodesInChaos Oct 29 '11 at 19:12
0
int compare (void*p, void*q) { return (p==q)?0:((char*)p < (char*)q)?-1:1; }
Mateen Ulhaq
  • 24,552
  • 19
  • 101
  • 135
Basile Starynkevitch
  • 223,805
  • 18
  • 296
  • 547
0

If you happen to compare addresses on the stack and heap I would say it's quite possible that the difference could be larger than that (since typically the heap grows up from the bottom and the stack grows down from the top).

Instead of returning int, return ptrdiff_t which as its name implies is an integral type big enough to hold pointer differences. You do still have to cast, although I chose char* instead of int because it lets you use static_cast:

ptrdiff_t compare(void* A, void* B) { return static_cast<char*>(A) - static_cast<char*>(B); }

Finally if you're using this with C's qsort, just take the simple way: Use std::sort which can use a single < and doesn't need to do any subtraction at all!

Mark B
  • 95,107
  • 10
  • 109
  • 188
  • `ptrdiff_t compare(void* A, void* B) { return (char*)A - (char*)B; }` – Keith Thompson Oct 28 '11 at 18:19
  • @Ben Voigt Updated to use difference of `char*`. – Mark B Oct 28 '11 at 18:22
  • "A subtraction of two pointers is only granted to have a valid defined value for pointers to elements of the same array (or for the element just past the last in the array)." While this is true for all pointer comparisons this will fail in more situations in practice than a direct pointer comparison. – CodesInChaos Oct 29 '11 at 19:16
0

First of all: Pointer comparisons are only defined if the pointers point into the same array. But I assume you don't care as long as it works in practice.

On x86 and AMD64 comparisons between pointers with < and > will most likely work in practice. But difference based methods can fail due to int-overflows.

There is an existing function named "Compare," which is

int compare(void* A, void* B) { return (int)A - (int)B; }

This is already broken on 32 bit systems where pointers the high bit set exist. This will not only lead to a strange ordering, but violates the transitivity property an order needs. On windows this can happen with applications that use /LARGEADDRESSAWARE (which allows 3GB of user mode address space). I don't know enough about Linux to know if it can happen there.

So you should use Ben Voigt's code even on 32 bit systems.

Community
  • 1
  • 1
CodesInChaos
  • 106,488
  • 23
  • 218
  • 262