[m-rev.] diff: fix user events seg fault
Mark Brown
mark at csse.unimelb.edu.au
Wed Apr 4 11:08:39 AEST 2007
Hi,
This fixes the bug that has been causing the G12 trace goals branch to
fail in some cases.
I considered alleviating the fixed limit by making it configurable, either
when compiling the compiler or later. I decided that it wasn't worth it,
because the chief problem with the fixed limit is knowing of its existence.
If you know that a foreign proc needs more than the fixed limit then it is
no harder to refactor the code, as in this change, than to reconfigure the
compiler or application with a new limit.
Cheers,
Mark.
Estimated hours taken: 16
Branches: main
Fix failing user event test cases for asm_fast/x86. The problem was that
the implementation of read_specs_file, in foreign C code, was using more
stack space than the (undocumented) fixed limit allowed.
doc/reference_manual.texi:
Document the limit on the size of local variables in C code.
compiler/prog_event.m:
Use MR_make_string to put strings on the Mercury heap, rather
than using our own buffers. Aside from simplfying the code,
this avoids going over the fixed stack frame size limit.
Factor the implementation of read_specs_file so that matching
save/restore, open/close and malloc/free pairs are close together.
runtime/mercury_engine.c:
Change the fixed limit to the still voodoo but slightly less
mystifying value of 10kB.
Index: compiler/prog_event.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/prog_event.m,v
retrieving revision 1.10
diff -u -r1.10 prog_event.m
--- compiler/prog_event.m 23 Feb 2007 06:35:55 -0000 1.10
+++ compiler/prog_event.m 3 Apr 2007 10:55:13 -0000
@@ -154,6 +154,13 @@
"
#include ""mercury_event_spec.h""
#include <stdio.h>
+
+MR_String read_specs_file_2(MR_Code *proc_label, MR_String specs_file_name,
+ MR_String term_file_name);
+MR_String read_specs_file_3(MR_Code *proc_label, MR_String specs_file_name,
+ MR_String term_file_name, int spec_fd);
+MR_String read_specs_file_4(MR_Code *proc_label, MR_String specs_file_name,
+ MR_String term_file_name, int spec_fd, size_t size, char *spec_buf);
").
:- pragma foreign_proc("C",
@@ -161,95 +168,109 @@
_IO0::di, _IO::uo),
[will_not_call_mercury, promise_pure, tabled_for_io, thread_safe],
"
- int spec_fd;
- FILE *term_fp;
+ /*
+ ** We need to save/restore MR_hp so that we can allocate the return
+ ** value on Mercury's heap if necessary.
+ */
+ MR_save_transient_hp();
+ Problem = read_specs_file_2(MR_PROC_LABEL, SpecsFileName, TermFileName);
+ MR_restore_transient_hp();
+").
+
+:- pragma foreign_code("C", "
+
+MR_String
+read_specs_file_2(MR_Code *proc_label, MR_String specs_file_name,
+ MR_String term_file_name)
+{
+ int spec_fd;
+ MR_String problem;
/*
- ** Using snprint instead of sprintf below would be very slightly safer,
- ** but unfortunately enabling the declaration of snprintf in the system's
- ** header files requires finding the right #defines, and this set of
- ** #defines is very system-dependent. Even having MR_HAVE_SNPRINTF set
- ** is no guarantee that calling snprintf won't lead to a warning from
- ** the C compiler.
- **
** There are race conditions between opening the file, stat'ing the file
** and reading the contents of the file, but the Unix API doesn't really
** allow these race conditions to be resolved.
*/
- spec_fd = open(SpecsFileName, O_RDONLY);
+ spec_fd = open(specs_file_name, O_RDONLY);
if (spec_fd < 0) {
- char buf[4096];
+ problem = MR_make_string(proc_label, ""could not open %s: %s"",
+ specs_file_name, strerror(errno));
+ } else {
+ problem = read_specs_file_3(proc_label, specs_file_name,
+ term_file_name, spec_fd);
+ (void) close(spec_fd);
+ }
+ return problem;
+}
- sprintf(buf, ""could not open %s: %s"",
- SpecsFileName, strerror(errno));
- MR_make_aligned_string_copy(Problem, buf);
+MR_String
+read_specs_file_3(MR_Code *proc_label, MR_String specs_file_name,
+ MR_String term_file_name, int spec_fd)
+{
+ struct stat stat_buf;
+ MR_String problem;
+
+ if (fstat(spec_fd, &stat_buf) != 0) {
+ problem = MR_make_string(proc_label, ""could not stat %s"",
+ specs_file_name);
} else {
- struct stat stat_buf;
+ char *spec_buf;
- if (fstat(spec_fd, &stat_buf) != 0) {
- char buf[4096];
+ spec_buf = malloc(stat_buf.st_size + 1);
+ if (spec_buf == NULL) {
+ problem = MR_make_string(proc_label,
+ ""could not allocate memory for a copy of %s"",
+ specs_file_name);
+ } else {
+ problem = read_specs_file_4(proc_label, specs_file_name,
+ term_file_name, spec_fd, stat_buf.st_size, spec_buf);
+ free(spec_buf);
+ }
+ }
+ return problem;
+}
+
+MR_String
+read_specs_file_4(MR_Code *proc_label, MR_String specs_file_name,
+ MR_String term_file_name, int spec_fd, size_t size, char *spec_buf)
+{
+ size_t num_bytes_read;
+ MR_String problem;
+
+ num_bytes_read = read(spec_fd, spec_buf, size);
+ if (num_bytes_read != size) {
+ problem = MR_make_string(proc_label, ""could not read in %s"",
+ specs_file_name);
+ } else {
+ MR_EventSet event_set;
- sprintf(buf, ""could not stat %s"", SpecsFileName);
- MR_make_aligned_string_copy(Problem, buf);
+ /* NULL terminate the string we have read in. */
+ spec_buf[num_bytes_read] = '\\0';
+ event_set = MR_read_event_set(specs_file_name, spec_buf);
+ if (event_set == NULL) {
+ problem = MR_make_string(proc_label, ""could not parse %s"",
+ specs_file_name);
} else {
- char *spec_buf;
+ FILE *term_fp;
- spec_buf = malloc(stat_buf.st_size + 1);
- if (spec_buf == NULL) {
- char buf[4096];
-
- sprintf(buf, ""could not allocate memory for a copy of %s"",
- SpecsFileName);
- MR_make_aligned_string_copy(Problem, buf);
+ term_fp = fopen(term_file_name, ""w"");
+ if (term_fp == NULL) {
+ problem = MR_make_string(proc_label, ""could not open %s: %s"",
+ term_file_name, strerror(errno));
} else {
- size_t num_bytes_read;
-
- num_bytes_read = read(spec_fd, spec_buf, stat_buf.st_size);
- if (num_bytes_read != stat_buf.st_size) {
- char buf[4096];
-
- sprintf(buf, ""could not read in %s"", SpecsFileName);
- MR_make_aligned_string_copy(Problem, buf);
- } else {
- MR_EventSet event_set;
-
- /* NULL terminate the string we have read in. */
- spec_buf[num_bytes_read] = '\\0';
- event_set = MR_read_event_set(SpecsFileName, spec_buf);
- if (event_set == NULL) {
- char buf[4096];
-
- sprintf(buf, ""could not parse %s"", SpecsFileName);
- MR_make_aligned_string_copy(Problem, buf);
- } else {
- term_fp = fopen(TermFileName, ""w"");
- if (term_fp == NULL) {
- char buf[4096];
-
- sprintf(buf, ""could not open %s: %s"",
- TermFileName, strerror(errno));
- MR_make_aligned_string_copy(Problem, buf);
- } else {
- MR_print_event_set(term_fp, event_set);
- fclose(term_fp);
-
- /*
- ** Our caller tests Problem against the
- ** empty string, not NULL.
- */
-
- MR_make_aligned_string_copy(Problem, """");
- }
- }
- }
+ MR_print_event_set(term_fp, event_set);
+ fclose(term_fp);
- free(spec_buf);
+ /*
+ ** Our caller tests Problem against the empty string, not NULL.
+ */
+ problem = MR_make_string(proc_label, """");
}
}
-
- (void) close(spec_fd);
}
+ return problem;
+}
").
%-----------------------------------------------------------------------------%
Index: doc/reference_manual.texi
===================================================================
RCS file: /home/mercury1/repository/mercury/doc/reference_manual.texi,v
retrieving revision 1.388
diff -u -r1.388 reference_manual.texi
--- doc/reference_manual.texi 6 Mar 2007 04:22:38 -0000 1.388
+++ doc/reference_manual.texi 3 Apr 2007 11:19:47 -0000
@@ -7068,9 +7068,15 @@
to their Mercury types, as determined by the rules specified in
@ref{C data passing conventions}.
-The C code fragment may declare local variables, but it should not
-declare any labels or static variables unless there is also a Mercury
- at samp{pragma no_inline} declaration for the procedure.
+The C code fragment may declare local variables,
+up to a total size of 10kB for the procedure.
+If a procedure requires more than this for its local variables,
+the code can be moved into a separate function
+(defined in a @samp{pragma foreign_code} declaration, for example).
+
+The C code fragment should not declare any labels or static variables
+unless there is also a Mercury @samp{pragma no_inline} declaration
+for the procedure.
The reason for this is that otherwise the Mercury implementation may
inline the procedure by duplicating the C code fragment for each call.
If the C code fragment declared a static variable, inlining it in this
Index: runtime/mercury_engine.c
===================================================================
RCS file: /home/mercury1/repository/mercury/runtime/mercury_engine.c,v
retrieving revision 1.57
diff -u -r1.57 mercury_engine.c
--- runtime/mercury_engine.c 6 Mar 2007 03:35:28 -0000 1.57
+++ runtime/mercury_engine.c 3 Apr 2007 11:24:10 -0000
@@ -27,7 +27,12 @@
#ifdef MR_USE_GCC_NONLOCAL_GOTOS
- #define LOCALS_SIZE 10024 /* space to reserve for local vars */
+ /*
+ ** Space to reserve for local vars. If this parameter is modified
+ ** then the reference manual will also need to be updated.
+ */
+ #define LOCALS_SIZE 10240
+
#define MAGIC_MARKER 187 /* a random character */
#define MAGIC_MARKER_2 142 /* another random character */
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to: mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions: mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------
More information about the reviews
mailing list