[distcc] Ada support in distcc
Martin Pool
mbp at sourcefrog.net
Mon Feb 20 19:03:53 MST 2012
Hi, thanks for the patch.
I think I recall having to write the dining philosophers in Ada for a
uni assignment many years ago :)
I have some comments on the diff but they don't mean I'm not happy to have this.
It's good to have some smoke tests for Ada. I do wonder if the
example code is a bit larger than it really has to be to make sure
that it compiles correctly. Probably just 'hello world' would do
about as well, unless exercising eg cursor movement is particularly
useful to be sure things link correctly. I suppose disk is cheap, but
some people will download this over slow connections and there's no
point making things much bigger than they have to be. Also, the more
code, the more likely it will cause spurious ada-environment-dependent
failures.
The cgpr files seem to have hardcoded paths and options. What uses them?
[tweak] You seem to be mixing in tabs and spaces, which scrambles the
diff. Please reindent everything to just use spaces and to be
consistent with the current style (4-space columns etc). Also, you're
sometimes putting a space between the function and the arguments;
please don't.
+
/**
+ * Transmit environment information
+ * - only sent to include server, not distcc server
+ * - needed for ADA_PRJ_INCLUDE_FILE, CPATH, C_INCLUDE_PATH,
CPLUS_INCLUDE_PATH
+ **/
+int dcc_x_env(int fd, unsigned int n_env, const char **envnames)
+{
+ char *env_value;
+ char *envname;
+
+ if (dcc_x_token_int(fd, "NENV", (unsigned) n_env))
+ return EXIT_PROTOCOL_ERROR;
+
+ for (; *envnames != NULL; ++envnames) {
+ envname = (char*) *envnames;
+ // make sure at least an empty string is sent
+ asprintf (&env_value, getenv(envname) ? getenv(envname) : "");
+ if (dcc_x_token_string(fd, "NAME", envname) ||
+ dcc_x_token_string(fd, "ENVI", env_value)) {
+ printf("client failed to send env %s = %s \n", envname, env_value);
+ free (env_value);
+ return -1;
+ }
+ free (env_value);
+ }
+ return 0;
+}
[fix] The n_env is redundant with looking for the null; this will be
less likely to go wrong if you just count the number of items within
this function, before you send it.
It looks like this is going to be an incompatible protocol break, so
people will need to upgrade both clients and servers together? That's
ok, but it ought to be clearly called out in the news.
There seems to be some mojibake in the news file.
[fix] There ought to be a manpage update for this, at least mentioning
that it _can_ do Ada, and the new config options.
Maybe you should mention, in the readme, something about how ada
support works, since most people who might want to hack on this (or
review your patch ;-) won't have used it much.
[fix] You're using printf to stdout for some errors; is that right?
+//static int tweak_ada_gnatem_file (char *filename, const char *root_dir)
If this is going to be useful to anyone else, make it configurable,
otherwise delete it.
+
+ // ada does not use the -MMD, -MF options for dependency
generation; skip for ada
+ if (!source_is_ada)
+ {
it seems like those are not really "not ada" but rather
source_is_cish. more generally, let's introduce a concept (probably
an enum) of say 'language family', that can be C, Ada, maybe other
things. That will give people a clue how to add non-gcc support.
+ int Ada_compile = FALSE;
lowercase locals please
It would be good to run your version under Valgrind, for both C and
Ada, to see if there are any memory handling bugs.
===================================================================
--- src/compile.c (revision 755)
+++ src/compile.c (working copy)
@@ -220,18 +220,6 @@
host->cpp_where,
&host->protover);
}
- /* Environment variables CPATH and two friends are hidden ways of passing
- * -I's. Beware! */
- if (getenv("CPATH") || getenv("C_INCLUDE_PATH")
- || getenv("CPLUS_INCLUDE_PATH")) {
- rs_log_warning("cannot use distcc_pump with any of environment"
- " variables CPATH, C_INCLUDE_PATH or CPLUS_INCLUDE_PATH"
- " set, preprocessing locally");
- host->cpp_where = DCC_CPP_ON_CLIENT;
- dcc_get_protover_from_features(host->compr,
- host->cpp_where,
- &host->protover);
- }
}
Can you explain why you removed this?
+
+ /* Fallback to non-pump mode for files listed in the env variable
DISTCC_NO_PUMP */
+ /* example: export DISTCC_NO_PUMP="file1.c file2.ada file3.cpp" */
+ char *skip_list = NULL;
+ char *skip_file = NULL;
+ asprintf (&skip_list, getenv("DISTCC_NO_PUMP"));
Why this line?
+ for (skip_file = strtok(skip_list, " "); skip_file != NULL; ) {
+ if (strcmp (basename(input_fname), basename(skip_file)) == 0) {
+ host->cpp_where = DCC_CPP_ON_CLIENT;
+ dcc_get_protover_from_features(host->compr,
+ host->cpp_where,
+ &host->protover);
+ break;
+ }
+ skip_file = strtok(NULL, " ");
+ }
+ free(skip_list);
Please put tabs in the code as \t not ascii 9, or it will definitely get broken.
In some ways it would be cleaner to just pass the option for the
compiler command that's specifically having trouble. I can see how it
may be hard to get a makefile to do that and you want distcc to match
on the basename....
strtok is an awful api; maybe there are better alternatives already in distcc?
@@ -786,6 +794,20 @@
free(server_side_argv);
}
free(discrepancy_filename);
+ free(deps_fname);
+//- free(files);
+
+// free(input_fname);
+// free(output_fname);
+//- free(cpp_fname);
+// free(*files);
+// free(*server_side_argv);
+ free(host);
+ free(server_stderr_fname);
+
+
+
+
return ret;
}
why this? It smells a bit like there are unknown memory bugs.
@@ -234,6 +235,8 @@
int dcc_is_source(const char *sfile)
{
const char *dot, *ext;
+ char lower_ext[] = "xxx";
+ char *c;
dot = dcc_find_extension_const(sfile);
if (!dot)
return 0;
@@ -268,13 +271,45 @@
case 'S':
return !strcmp(ext, "S");
#endif
- default:
+ // TODO: how to make this accommodate all ada suffixes
+ case 'a':
+ case 'A':
+ // match all cases
+ strncpy (lower_ext, ext, 3);
+ for (c=lower_ext; *c != 0; *c = tolower(*c), c++);
+ return !strcmp(lower_ext,"ada")
+ || !strcmp(lower_ext,"adb")
+ || !strcmp(lower_ext,"ads");
+ default:
return 0;
}
}
[fix] This is writing over a constant string, lower_ext. I'm
surprised it's not crashing for you, and I'm pretty sure it will crash
on some systems. Why not just use strcasecmp?
+/**
+ * Is the file ada source?
+ **/
+int dcc_is_ada(const char *sfile)
+{
+ const char *dot, *ext;
+ char lower_ext[] = "xxx";
+ char *c;
+ dot = dcc_find_extension_const(sfile);
+ if (!dot)
+ return 0;
+ ext = dot+1;
+ // TODO: how to make this accommodate all ada suffixes?
+ // => read gnatec file
+ // for now, match all cases
+ strncpy (lower_ext, ext, 3);
+ for (c=lower_ext; *c != 0; *c = tolower(*c), c++);
+ return !strcmp(lower_ext,"ada")
+ || !strcmp(lower_ext,"adb")
+ || !strcmp(lower_ext,"ads");
+}
+
+
/**
This seems redundant with the code above, and it has the same serious
bug. Please put spaces between parameters like the rest of the code.
Index: src/cleanup.c
===================================================================
--- src/cleanup.c (revision 755)
+++ src/cleanup.c (working copy)
@@ -100,7 +100,7 @@
* if that fails, try removing is as a file.
* Report the error from removing-as-a-file
* if both fail. */
- if ((rmdir(cleanups[i]) == -1) &&
+ if ((rmdir(cleanups[i]) == -1) &&
(unlink(cleanups[i]) == -1) &&
(errno != ENOENT)) {
rs_log_notice("cleanup %s failed: %s", cleanups[i],
it was fine where it was :)
I think this would be safer with some unit tests for the new ada logic.
@@ -186,6 +189,35 @@
}
for (i = 0; (a = argv[i]); i++) {
+
+ // Special case for gnat ada, where .ali files are
+ // always generated, and are always named <source>.ali
+ if (strstr(a, "-gnat") != NULL) {
If this is saying "if there's any argument containing '-gnat' it's an
ada compile" then I think it's wrong.
+ *needs_dotd = 1;
+ *sets_dotd_target = 1;
+ rs_log_info("found gnat");
+
+ // find source filename and mod the extension
+ for (j = 0; (a = argv[j]); j++) {
+ if (dcc_is_source (a)) {
+ rs_log_info("found source name: %s", a);
+ if (asprintf (&temp_dotd_fname, "%s---", a) ==-1) {
+ return EXIT_OUT_OF_MEMORY;
+ }
is this a weird way of allocating a 3-byte longer buffer? why?
+ // this changes the file extension to .ali
+ dcc_output_from_source(temp_dotd_fname, ".ali", dotd_fname);
+ free (temp_dotd_fname);
+ rs_log_info("ali=%s", *dotd_fname);
+ return 0;
+ }
+ }
+ // could not find the source file???
+ rs_log_error("Cound not find the gnat source file to set
the .ali filename");
+ return EXIT_DISTCC_FAILED;
+ }
+
+
+
you have several function calls such as asprintf, whose return values
need to be checked but aren't.
+ //TODO: - utime overflows in the year 2038
:-)
So this could probably do with another look over the specific ada
manipulations. I do suspect there is some dodgy pointer manipulation
there and running some tests under valgrind and writing some unit
tests would be a good way to shake that out.
regards,
Martin
More information about the distcc
mailing list