From 1396ac86dea5fccab800e4b25fdb5319381891eb Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Thu, 18 Jul 2013 18:24:35 -0700 Subject: [PATCH] Fix obscure porting bug with varargs functions. The code assumed that int is treated like ptrdiff_t in a vararg function, which is not a portable assumption. There was a similar -- though these days less likely -- porting problem with various assumptions that pointers of different types all smell the same as far as vararg functions is conserved. To make this problem less likely in the future, redo the API to use varargs functions. * alloc.c (make_save_value): Remove this vararg function. All uses changed to ... (make_save_int_int_int, make_save_obj_obj_obj_obj) (make_save_ptr_int, make_save_funcptr_ptr_obj, make_save_memory): New functions. (make_save_ptr): Rename from make_save_pointer, for consistency with the above. Define only on platforms that need it. All uses changed. --- src/ChangeLog | 17 +++++++++ src/alloc.c | 97 ++++++++++++++++++++++++++++++--------------------- src/editfns.c | 5 ++- src/fileio.c | 3 +- src/font.c | 2 +- src/ftfont.c | 2 +- src/keymap.c | 4 +-- src/lisp.h | 43 ++++++++++++----------- src/nsterm.m | 2 +- src/w32fns.c | 2 +- src/xmenu.c | 3 +- 11 files changed, 108 insertions(+), 72 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 81b23bccc8..73fc64f37c 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,20 @@ +2013-07-19 Paul Eggert + + Fix obscure porting bug with varargs functions. + The code assumed that int is treated like ptrdiff_t in a vararg + function, which is not a portable assumption. There was a similar + -- though these days less likely -- porting problem with various + assumptions that pointers of different types all smell the same as + far as vararg functions is conserved. To make this problem less + likely in the future, redo the API to use varargs functions. + * alloc.c (make_save_value): Remove this vararg function. + All uses changed to ... + (make_save_int_int_int, make_save_obj_obj_obj_obj) + (make_save_ptr_int, make_save_funcptr_ptr_obj, make_save_memory): + New functions. + (make_save_ptr): Rename from make_save_pointer, for consistency with + the above. Define only on platforms that need it. All uses changed. + 2013-07-18 Paul Eggert * keyboard.c: Try to fix typos in previous change. diff --git a/src/alloc.c b/src/alloc.c index 39f6a82b13..11245741bd 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -3342,62 +3342,81 @@ verify (((SAVE_INTEGER | SAVE_POINTER | SAVE_FUNCPOINTER | SAVE_OBJECT) >> SAVE_SLOT_BITS) == 0); -/* Return a Lisp_Save_Value object with the data saved according to - DATA_TYPE. DATA_TYPE should be one of SAVE_TYPE_INT_INT, etc. */ +/* Return Lisp_Save_Value objects for the various combinations + that callers need. */ Lisp_Object -make_save_value (enum Lisp_Save_Type save_type, ...) +make_save_int_int_int (ptrdiff_t a, ptrdiff_t b, ptrdiff_t c) { - va_list ap; - int i; Lisp_Object val = allocate_misc (Lisp_Misc_Save_Value); struct Lisp_Save_Value *p = XSAVE_VALUE (val); + p->save_type = SAVE_TYPE_INT_INT_INT; + p->data[0].integer = a; + p->data[1].integer = b; + p->data[2].integer = c; + return val; +} - eassert (0 < save_type - && (save_type < 1 << (SAVE_TYPE_BITS - 1) - || save_type == SAVE_TYPE_MEMORY)); - p->save_type = save_type; - va_start (ap, save_type); - save_type &= ~ (1 << (SAVE_TYPE_BITS - 1)); - - for (i = 0; save_type; i++, save_type >>= SAVE_SLOT_BITS) - switch (save_type & ((1 << SAVE_SLOT_BITS) - 1)) - { - case SAVE_POINTER: - p->data[i].pointer = va_arg (ap, void *); - break; - - case SAVE_FUNCPOINTER: - p->data[i].funcpointer = va_arg (ap, voidfuncptr); - break; - - case SAVE_INTEGER: - p->data[i].integer = va_arg (ap, ptrdiff_t); - break; +Lisp_Object +make_save_obj_obj_obj_obj (Lisp_Object a, Lisp_Object b, Lisp_Object c, + Lisp_Object d) +{ + Lisp_Object val = allocate_misc (Lisp_Misc_Save_Value); + struct Lisp_Save_Value *p = XSAVE_VALUE (val); + p->save_type = SAVE_TYPE_OBJ_OBJ_OBJ_OBJ; + p->data[0].object = a; + p->data[1].object = b; + p->data[2].object = c; + p->data[3].object = d; + return val; +} - case SAVE_OBJECT: - p->data[i].object = va_arg (ap, Lisp_Object); - break; +#if defined HAVE_NS || defined DOS_NT +Lisp_Object +make_save_ptr (void *a) +{ + Lisp_Object val = allocate_misc (Lisp_Misc_Save_Value); + struct Lisp_Save_Value *p = XSAVE_VALUE (val); + p->save_type = SAVE_POINTER; + p->data[0].pointer = a; + return val; +} +#endif - default: - emacs_abort (); - } +Lisp_Object +make_save_ptr_int (void *a, ptrdiff_t b) +{ + Lisp_Object val = allocate_misc (Lisp_Misc_Save_Value); + struct Lisp_Save_Value *p = XSAVE_VALUE (val); + p->save_type = SAVE_TYPE_PTR_INT; + p->data[0].pointer = a; + p->data[1].integer = b; + return val; +} - va_end (ap); +Lisp_Object +make_save_funcptr_ptr_obj (void (*a) (void), void *b, Lisp_Object c) +{ + Lisp_Object val = allocate_misc (Lisp_Misc_Save_Value); + struct Lisp_Save_Value *p = XSAVE_VALUE (val); + p->save_type = SAVE_TYPE_FUNCPTR_PTR_OBJ; + p->data[0].funcpointer = a; + p->data[1].pointer = b; + p->data[2].object = c; return val; } -/* Save just one C pointer. record_unwind_protect_ptr is simpler and - faster than combining this with record_unwind_protect, but - occasionally this function is useful for other reasons. */ +/* Return a Lisp_Save_Value object that represents an array A + of N Lisp objects. */ Lisp_Object -make_save_pointer (void *pointer) +make_save_memory (Lisp_Object *a, ptrdiff_t n) { Lisp_Object val = allocate_misc (Lisp_Misc_Save_Value); struct Lisp_Save_Value *p = XSAVE_VALUE (val); - p->save_type = SAVE_POINTER; - p->data[0].pointer = pointer; + p->save_type = SAVE_TYPE_MEMORY; + p->data[0].pointer = a; + p->data[1].integer = n; return val; } diff --git a/src/editfns.c b/src/editfns.c index a4dea203a2..50bde90788 100644 --- a/src/editfns.c +++ b/src/editfns.c @@ -838,9 +838,8 @@ This function does not move point. */) Lisp_Object save_excursion_save (void) { - return make_save_value - (SAVE_TYPE_OBJ_OBJ_OBJ_OBJ, - Fpoint_marker (), + return make_save_obj_obj_obj_obj + (Fpoint_marker (), /* Do not copy the mark if it points to nowhere. */ (XMARKER (BVAR (current_buffer, mark))->buffer ? Fcopy_marker (BVAR (current_buffer, mark), Qnil) diff --git a/src/fileio.c b/src/fileio.c index 5fe359d58b..a19fcd9f66 100644 --- a/src/fileio.c +++ b/src/fileio.c @@ -4215,8 +4215,7 @@ by calling `format-decode', which see. */) to be signaled after decoding the text we read. */ nbytes = internal_condition_case_1 (read_non_regular, - make_save_value (SAVE_TYPE_INT_INT_INT, (ptrdiff_t) fd, - inserted, trytry), + make_save_int_int_int (fd, inserted, trytry), Qerror, read_non_regular_quit); if (NILP (nbytes)) diff --git a/src/font.c b/src/font.c index 80b4b76c4e..124d5f9bd9 100644 --- a/src/font.c +++ b/src/font.c @@ -1861,7 +1861,7 @@ otf_open (Lisp_Object file) else { otf = STRINGP (file) ? OTF_open (SSDATA (file)) : NULL; - val = make_save_pointer (otf); + val = make_save_ptr (otf); otf_list = Fcons (Fcons (file, val), otf_list); } return otf; diff --git a/src/ftfont.c b/src/ftfont.c index 7c9534d5ae..10090cb3bd 100644 --- a/src/ftfont.c +++ b/src/ftfont.c @@ -393,7 +393,7 @@ ftfont_lookup_cache (Lisp_Object key, enum ftfont_cache_for cache_for) cache_data = xmalloc (sizeof *cache_data); cache_data->ft_face = NULL; cache_data->fc_charset = NULL; - val = make_save_value (SAVE_TYPE_PTR_INT, cache_data, 0); + val = make_save_ptr_int (cache_data, 0); cache = Fcons (Qnil, val); Fputhash (key, cache, ft_face_cache); } diff --git a/src/keymap.c b/src/keymap.c index e1268c8a06..d13a627434 100644 --- a/src/keymap.c +++ b/src/keymap.c @@ -617,8 +617,8 @@ map_keymap_internal (Lisp_Object map, } else if (CHAR_TABLE_P (binding)) map_char_table (map_keymap_char_table_item, Qnil, binding, - make_save_value (SAVE_TYPE_FUNCPTR_PTR_OBJ, - (voidfuncptr) fun, data, args)); + make_save_funcptr_ptr_obj ((voidfuncptr) fun, data, + args)); } UNGCPRO; return tail; diff --git a/src/lisp.h b/src/lisp.h index 518de9db0f..254ead231b 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -441,8 +441,7 @@ enum Lisp_Fwd_Type displayed to users. These are Lisp_Save_Value, a Lisp_Misc subtype; and PVEC_OTHER, a kind of vectorlike object. The former is suitable for temporarily stashing away pointers and integers in - a Lisp object (see the existing uses of make_save_value and - XSAVE_VALUE). The latter is useful for vector-like Lisp objects + a Lisp object. The latter is useful for vector-like Lisp objects that need to be used as part of other objects, but which are never shown to users or Lisp code (search for PVEC_OTHER in xterm.c for an example). @@ -1815,30 +1814,26 @@ enum Lisp_Save_Type This is mostly used to package C integers and pointers to call record_unwind_protect when two or more values need to be saved. - make_save_value lets you pack up to SAVE_VALUE_SLOTS integers, pointers, - function pointers or Lisp_Objects and conveniently get them back - with XSAVE_INTEGER, XSAVE_POINTER, XSAVE_FUNCPOINTER, and - XSAVE_OBJECT macros: + For example: ... struct my_data *md = get_my_data (); - Lisp_Object my_object = get_my_object (); - record_unwind_protect - (my_unwind, make_save_value (SAVE_TYPE_PTR_OBJ, md, my_object)); + ptrdiff_t mi = get_my_integer (); + record_unwind_protect (my_unwind, make_save_ptr_int (md, mi)); ... Lisp_Object my_unwind (Lisp_Object arg) { struct my_data *md = XSAVE_POINTER (arg, 0); - Lisp_Object my_object = XSAVE_OBJECT (arg, 1); + ptrdiff_t mi = XSAVE_INTEGER (arg, 1); ... } If ENABLE_CHECKING is in effect, XSAVE_xxx macros do type checking of the saved objects and raise eassert if type of the saved object doesn't match the type which is extracted. In the example above, XSAVE_INTEGER (arg, 2) - or XSAVE_OBJECT (arg, 0) are wrong because nothing was saved in slot 2 and - Lisp_Object was saved in slot 1 of ARG. */ + and XSAVE_OBJECT (arg, 0) are wrong because nothing was saved in slot 2 and + slot 0 is a pointer. */ typedef void (*voidfuncptr) (void); @@ -1848,12 +1843,13 @@ struct Lisp_Save_Value unsigned gcmarkbit : 1; int spacer : 32 - (16 + 1 + SAVE_TYPE_BITS); - /* DATA[N] may hold up to SAVE_VALUE_SLOTS entries. The type of - V's Ith entry is given by save_type (V, I). E.g., if save_type - (V, 3) == SAVE_INTEGER, V->data[3].integer is in use. + /* V->data may hold up to SAVE_VALUE_SLOTS entries. The type of + V's data entries are determined by V->save_type. E.g., if + V->save_type == SAVE_TYPE_PTR_OBJ, V->data[0] is a pointer, + V->data[1] is an integer, and V's other data entries are unused. - If SAVE_TYPE == SAVE_TYPE_MEMORY, DATA[0].pointer is the address of - a memory area containing DATA[1].integer potential Lisp_Objects. */ + If V->save_type == SAVE_TYPE_MEMORY, V->data[0].pointer is the address of + a memory area containing V->data[1].integer potential Lisp_Objects. */ ENUM_BF (Lisp_Save_Type) save_type : SAVE_TYPE_BITS; union { void *pointer; @@ -3580,8 +3576,15 @@ extern bool abort_on_gc; extern Lisp_Object make_float (double); extern void display_malloc_warning (void); extern ptrdiff_t inhibit_garbage_collection (void); -extern Lisp_Object make_save_value (enum Lisp_Save_Type, ...); -extern Lisp_Object make_save_pointer (void *); +extern Lisp_Object make_save_int_int_int (ptrdiff_t, ptrdiff_t, ptrdiff_t); +extern Lisp_Object make_save_obj_obj_obj_obj (Lisp_Object, Lisp_Object, + Lisp_Object, Lisp_Object); +extern Lisp_Object make_save_ptr (void *); +extern Lisp_Object make_save_ptr_int (void *, ptrdiff_t); +extern Lisp_Object make_save_ptr_ptr (void *, void *); +extern Lisp_Object make_save_funcptr_ptr_obj (void (*) (void), void *, + Lisp_Object); +extern Lisp_Object make_save_memory (Lisp_Object *, ptrdiff_t); extern void free_save_value (Lisp_Object); extern Lisp_Object build_overlay (Lisp_Object, Lisp_Object, Lisp_Object); extern void free_marker (Lisp_Object); @@ -4314,7 +4317,7 @@ extern void *record_xmalloc (size_t); { \ Lisp_Object arg_; \ buf = xmalloc ((nelt) * word_size); \ - arg_ = make_save_value (SAVE_TYPE_MEMORY, buf, nelt); \ + arg_ = make_save_memory (buf, nelt); \ sa_must_free = 1; \ record_unwind_protect (free_save_value, arg_); \ } \ diff --git a/src/nsterm.m b/src/nsterm.m index c91e68f37a..f3c35e95bf 100644 --- a/src/nsterm.m +++ b/src/nsterm.m @@ -3777,7 +3777,7 @@ ns_set_vertical_scroll_bar (struct window *window, } bar = [[EmacsScroller alloc] initFrame: r window: win]; - wset_vertical_scroll_bar (window, make_save_pointer (bar)); + wset_vertical_scroll_bar (window, make_save_ptr (bar)); } else { diff --git a/src/w32fns.c b/src/w32fns.c index 5d9200bdd7..675b716f3b 100644 --- a/src/w32fns.c +++ b/src/w32fns.c @@ -4916,7 +4916,7 @@ w32_monitor_enum (HMONITOR monitor, HDC hdc, RECT *rcMonitor, LPARAM dwData) { Lisp_Object *monitor_list = (Lisp_Object *) dwData; - *monitor_list = Fcons (make_save_pointer (monitor), *monitor_list); + *monitor_list = Fcons (make_save_ptr (monitor), *monitor_list); return TRUE; } diff --git a/src/xmenu.c b/src/xmenu.c index 1151dea440..6c0e3dd78a 100644 --- a/src/xmenu.c +++ b/src/xmenu.c @@ -2465,8 +2465,7 @@ xmenu_show (FRAME_PTR f, int x, int y, bool for_click, bool keymaps, XMenuActivateSetWaitFunction (x_menu_wait_for_event, FRAME_X_DISPLAY (f)); #endif - record_unwind_protect (pop_down_menu, - make_save_value (SAVE_TYPE_PTR_PTR, f, menu)); + record_unwind_protect (pop_down_menu, make_save_ptr_ptr (f, menu)); /* Help display under X won't work because XMenuActivate contains a loop that doesn't give Emacs a chance to process it. */ -- 2.20.1