Re: [PATCH v3] MSVC: fix t0040-parse-options crash
On Sun, Mar 30, 2014 at 10:29:04AM +0200, Andreas Schwab wrote: Junio C Hamano gits...@pobox.com writes: As OPT_SET_PTR() is about setting the pointer value to intptr_t defval, a follow-up patch on top of this fix (see attached) may not be a bad thing to have, but that patch alone will not fix this issue without dropping the unneeded and unwanted cast to unsigned long. Wouldn't it make sense to change defval into a union to avoid the cast? (The intptr_t type may be too narrow for other values to be put there.) The primary function of these structs is to capture the information found in brace initializers. Is it possible in C89 to initialize the second member of a union (I think in C99, you can use named initializers). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] MSVC: fix t0040-parse-options crash
Junio C Hamano gits...@pobox.com writes: As OPT_SET_PTR() is about setting the pointer value to intptr_t defval, a follow-up patch on top of this fix (see attached) may not be a bad thing to have, but that patch alone will not fix this issue without dropping the unneeded and unwanted cast to unsigned long. Wouldn't it make sense to change defval into a union to avoid the cast? (The intptr_t type may be too narrow for other values to be put there.) Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] MSVC: fix t0040-parse-options crash
On 64-bit MSVC, pointers are 64 bit but `long` is only 32.
Thus, casting string to `unsigned long`, which is redundand on other
platforms, throws away important bits and when later cast to `intptr_t`
results in corrupt pointer.
This patch fixes test-parse-options by replacing harming cast with
correct one.
Signed-off-by: Marat Radchenko ma...@slonopotamus.org
---
Aargh! Didn't notice that V2 introduced compilation warning. Take three.
test-parse-options.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test-parse-options.c b/test-parse-options.c
index 434e8b8..6f6c656 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -60,7 +60,7 @@ int main(int argc, char **argv)
OPT_STRING('o', NULL, string, str, get another string),
OPT_NOOP_NOARG(0, obsolete),
OPT_SET_PTR(0, default-string, string,
- set string to default, (unsigned long)default),
+ set string to default, (intptr_t)default),
OPT_STRING_LIST(0, list, list, str, add str to list),
OPT_GROUP(Magic arguments),
OPT_ARGUMENT(quux, means --quux),
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] MSVC: fix t0040-parse-options crash
Marat Radchenko ma...@slonopotamus.org writes:
diff --git a/test-parse-options.c b/test-parse-options.c
index 434e8b8..6f6c656 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -60,7 +60,7 @@ int main(int argc, char **argv)
OPT_STRING('o', NULL, string, str, get another string),
OPT_NOOP_NOARG(0, obsolete),
OPT_SET_PTR(0, default-string, string,
- set string to default, (unsigned long)default),
+ set string to default, (intptr_t)default),
Why doesn't OPT_SET_PTR take a pointer?
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] MSVC: fix t0040-parse-options crash
Am 29.03.2014 22:34, schrieb Andreas Schwab:
Marat Radchenko ma...@slonopotamus.org writes:
diff --git a/test-parse-options.c b/test-parse-options.c
index 434e8b8..6f6c656 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -60,7 +60,7 @@ int main(int argc, char **argv)
OPT_STRING('o', NULL, string, str, get another string),
OPT_NOOP_NOARG(0, obsolete),
OPT_SET_PTR(0, default-string, string,
- set string to default, (unsigned long)default),
+ set string to default, (intptr_t)default),
Why doesn't OPT_SET_PTR take a pointer?
Good question. Here's another: OPT_SET_PTR (and OPTION_SET_PTR) has
only ever been used by test-parse-options; can we remove it?
René
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] MSVC: fix t0040-parse-options crash
Marat Radchenko ma...@slonopotamus.org writes:
On 64-bit MSVC, pointers are 64 bit but `long` is only 32.
Thus, casting string to `unsigned long`, which is redundand on other
platforms, throws away important bits and when later cast to `intptr_t`
results in corrupt pointer.
This patch fixes test-parse-options by replacing harming cast with
correct one.
Signed-off-by: Marat Radchenko ma...@slonopotamus.org
---
Aargh! Didn't notice that V2 introduced compilation warning. Take three.
I am glad that I asked you to clarify, as I totally forgot that
there are L32P64 boxes.
I love it every time to see an attempt to describe why the solution
works clearly results in a better patch. It is not about writing
verbose log message; it is about thinking things through and clearly
cut to the core of the issue. Moving the string literal to a
separate variable to be used in the constructor in v1 was totally a
red-herring. Your updated log message makes it crystal clear that
using the correct typecast, not unsigned long but intptr_t, is
the core of the solution.
As OPT_SET_PTR() is about setting the pointer value to intptr_t defval,
a follow-up patch on top of this fix (see attached) may not be a bad
thing to have, but that patch alone will not fix this issue without
dropping the unneeded and unwanted cast to unsigned long.
Thanks.
test-parse-options.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test-parse-options.c b/test-parse-options.c
index 434e8b8..6f6c656 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -60,7 +60,7 @@ int main(int argc, char **argv)
OPT_STRING('o', NULL, string, str, get another string),
OPT_NOOP_NOARG(0, obsolete),
OPT_SET_PTR(0, default-string, string,
- set string to default, (unsigned long)default),
+ set string to default, (intptr_t)default),
OPT_STRING_LIST(0, list, list, str, add str to list),
OPT_GROUP(Magic arguments),
OPT_ARGUMENT(quux, means --quux),
parse-options.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/parse-options.h b/parse-options.h
index d670cb9..7a24d2e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -129,7 +129,7 @@ struct option {
#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
NULL, 1}
#define OPT_SET_PTR(s, l, v, h, p) { OPTION_SET_PTR, (s), (l), (v), NULL, \
- (h), PARSE_OPT_NOARG, NULL, (p) }
+ (h), PARSE_OPT_NOARG, NULL, (intptr_t)(p)
}
#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \
(h), PARSE_OPT_NOARG|PARSE_OPT_NONEG,
NULL, (i) }
#define OPT_INTEGER(s, l, v, h) { OPTION_INTEGER, (s), (l), (v), N_(n),
(h) }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
