Fix the remaining pointer-to-int-cast warnings #34

aitap · 2020-12-18T18:14:38Z

Hi again! Apologies for not bringing this up earlier.

Unfortunately, I missed some of the pointer ⇆ long cast warnings last time, so I squashed them now. Compilers really don't like casting between integers and pointers of different sizes, so they warn even when it's "safe" (e.g. casting a 32-bit pointer to a 64-bit integer). Here is a summary of changes I had to make:

PNT_ARITH_TYPE is now of the same size as void*, not always 64-bit. Strictly speaking, this could be a job for C99 uintptr_t (also, long long has also only been standardised in C99), but I see the value in keeping the code the way it is, defined purely in terms of integers with size modifiers.
append.c had to be changed to handle long longs and therefore the %ll size modifier. I added a few tests to make sure it works correctly. I modified the %x handler to print its argument as unsigned, which I think matches the standard. Sorry if you relied on %x being signed -- I can change that back.
Remaining casts to long for arithmetic purposes are replaced by casts to PNT_ARITH_TYPE.
Most cases where pointers were cast to long for formatting purposes are replaced by use of %p, except in chunk.c, where the size of the address range is printed (should it be signed or unsigned? both answers miss some unlikely corner cases), and env.c, where strings are formatted for the environment variable, which is public interface and shouldn't be changed, so I left the cast to long long and inserted a cast to PNT_ARITH_TYPE before that to silence the warning.

Compilers *really* dislike casts between pointers and integers not matching them in size, so the simple approach of casting pointers to the largest type available leads to warnings on 32-bit systems. Try harder to define PNT_ARITH_TYPE to a type with sizeof() == sizeof(void*) instead of opting for a 64-bit type everywhere. This should silence the warnings on 32-bit platforms.

Since we *may* need to use 64-bit arithmetic on machines where long is 32-bit (Windows), switch handle_decimal, append_long and append_ulong to long long arguments. Add tests to make sure that ULL constants are correctly formatted as signed decimal / unsigned decimal / hexadecimal (which should be unsigned). Ignore the octal, which for now is signed.

There were remaining cases where pointers are cast to long for arithmetic purposes. Unfortunately, long is 32-bit on 64-bit windows. Replace casts to long with casts to PNT_ARITH_TYPE, which should be of appropriate size.

_dmalloc_heap_high - _dmalloc_heap_low is a difference of two pointers, so it's unsafe to print as %ld, as longs are 32-bit on 64-bit Windows. Print it as an unsigned long long, which should be large enough even when _high is top of address space and _low is the bottom of address space. Alternatively, if _high < _low is a possibility, this should be printed as signed instead.

On 64-bit Windows, longs are 32-bit. In order to avoid warnings of casting between a pointer and an integer of a different size, first cast the pointer to PNT_ARITH_TYPE, which should be of appropriate size, then cast that to unsigned long long, which should be large enough for 32-bit and 64-bit pointers.

Since longs are 32-bit on 64-bit windows, casting them to long and printing as %#lx may lead to truncation. Instead, remove the casts and print them as %p. Hope nobody was parsing our pointer output. The alternative would be printing them as %#llx after casting to (unsigned long long)(PNT_ARITH_TYPE), two-stage cast required to avoid the warning for potentially casting to integer of a different type.

j256

Fix all of the %lx -> %p changes first? I need to think about the rest of it.

j256 · 2020-12-18T20:26:39Z

append.c

@@ -54,7 +54,7 @@
 /*
 * Internal method to process a long or int number and write into a buffer.
 */
-static	char	*handle_decimal(char *buf, char *limit, const long num, const int base)
+static	char	*handle_decimal(char *buf, char *limit, const long long num, const int base)


Decimal is either long or int. I don't support %ll or %ql right now. Since I'm printing pointers as %p I don't think this should be there.

I don't want to force support for long long either. It's not supported on some architectures.

Okay, keeping support for pre-C99 platforms is an entirely valid goal!

j256 · 2020-12-18T20:26:50Z

append.c

@@ -145,13 +145,13 @@ char	*append_string(char *dest, const char *limit, const char *value)
 * character will be added.  Variant of itoa() written by Lukas
 * Chmela which is released under GPLv3.
 */
-char	*append_long(char *dest, char *limit, long value, int base)
+char	*append_long(char *dest, char *limit, long long value, int base)


Yeah no this should be long.

j256 · 2020-12-18T20:27:44Z

append.c

@@ -380,8 +384,10 @@ char	*append_vformat(char *dest, char *limit, const char *format,
      } else if (ch == 's') {
 	value = va_arg(args, char *);
      } else if (ch == 'u') {
-	unsigned long num;
-	if (long_arg) {
+	unsigned long long num;


Yeah no, this should be long. If the arg is a long then we should get a long. Also, long long may not be portable.

j256 · 2020-12-18T20:28:17Z

chunk.c

@@ -1652,7 +1652,7 @@ static	int	check_used_slot(const skip_alloc_t *slot_p,
   */
  if (pnt_info.pi_valloc_b) {

-    if ((long)pnt_info.pi_user_start % BLOCK_SIZE != 0) {
+    if ((PNT_ARITH_TYPE)pnt_info.pi_user_start % BLOCK_SIZE != 0) {


j256 · 2020-12-18T20:30:57Z

chunk.c

 		  _dmalloc_heap_low, _dmalloc_heap_high,
-		  (unsigned long)_dmalloc_heap_high -
-		  (unsigned long)_dmalloc_heap_low);
+		  (unsigned long long)(unsigned PNT_ARITH_TYPE)_dmalloc_heap_high -


unsigned long long was a no-no back in the day because some 32 bit systems needed to use the high bit of the lower word (sic) meaning that they were 63 bit numbers. I kid you not. So I'd rather this just be a cast to PNT_ARITH_TYPE.

Wow, I didn't know that, thanks!

The problem is that it's hard to find a printf size modifier for PNT_ARITH_TYPE. If we want to be portable to the widest range of possible platforms (so probably ANSI C, and for most of them sizeof(void*)==sizeof(long) is probably true), PNT_ARITH_TYPE can't be larger than long, so printing it with %lx/%ld is okay. On the other hand, if we want this to work on 64-bit Windows, long is already too small, so even if we determine the correct (non-portable) type using a ./configure test, finding out how to print it is still a problem. I think that ANSI C has appropriately-sized ptrdiff_t and size_t, but no portable way of printing them, either.

There is probably no beautiful solution for this. I can work on a configure test for the format string where pointer-sized integers have to be printed without %p, but code readability may suffer.

j256 · 2020-12-18T20:31:24Z

append.c

@@ -324,7 +324,7 @@ char	*append_vformat(char *dest, char *limit, const char *format,
 	  }
 	}
      } else if (ch == 'l') {
-	long_arg = 1;
+	long_arg += 1;


This should be ++.

j256 · 2020-12-23T19:02:32Z

I think we can close this right @aitap ? Thanks again.

aitap · 2020-12-23T19:14:10Z

Yes, I think so. Just compiled with both i686-w64-mingw32 and x86_64-w64-mingw32 with CFLAGS=-Wall for good measure - no warnings, including make cxx and make shlib.

Do you want the commit that adds WinAPI VirtualAlloc support? It only adds a few lines in an #ifdef.

aitap added 6 commits December 18, 2020 19:42

Use PNT_ARITH_TYPE where pointers are cast to long

3a0933d

There were remaining cases where pointers are cast to long for arithmetic purposes. Unfortunately, long is 32-bit on 64-bit windows. Replace casts to long with casts to PNT_ARITH_TYPE, which should be of appropriate size.

j256 reviewed Dec 18, 2020

View reviewed changes

j256 mentioned this pull request Dec 23, 2020

Fixed some missing %p and some pointer arithmetic issues #35

Merged

j256 closed this Dec 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the remaining pointer-to-int-cast warnings #34

Fix the remaining pointer-to-int-cast warnings #34

aitap commented Dec 18, 2020

j256 left a comment

j256 Dec 18, 2020

j256 Dec 18, 2020

aitap Dec 23, 2020 •

edited

Loading

j256 Dec 18, 2020

j256 Dec 18, 2020 •

edited

Loading

j256 Dec 18, 2020

j256 Dec 18, 2020

aitap Dec 23, 2020

j256 Dec 18, 2020

j256 commented Dec 23, 2020

aitap commented Dec 23, 2020 •

edited

Loading

Fix the remaining pointer-to-int-cast warnings #34

Fix the remaining pointer-to-int-cast warnings #34

Conversation

aitap commented Dec 18, 2020

j256 left a comment

Choose a reason for hiding this comment

j256 Dec 18, 2020

Choose a reason for hiding this comment

j256 Dec 18, 2020

Choose a reason for hiding this comment

aitap Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

j256 Dec 18, 2020

Choose a reason for hiding this comment

j256 Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

j256 Dec 18, 2020

Choose a reason for hiding this comment

j256 Dec 18, 2020

Choose a reason for hiding this comment

aitap Dec 23, 2020

Choose a reason for hiding this comment

j256 Dec 18, 2020

Choose a reason for hiding this comment

j256 commented Dec 23, 2020

aitap commented Dec 23, 2020 • edited Loading

aitap Dec 23, 2020 •

edited

Loading

j256 Dec 18, 2020 •

edited

Loading

aitap commented Dec 23, 2020 •

edited

Loading