[PATCH]Assorted cleanup

[Andrew Black <ab...@roguewave.com>]

Greetings all.

Attached is a patch that aims to resolve a couple minor compile issues 
with the exec utility.

The first concern is the current link behavior, where the standard 
library is linked into all executables.  While this is normally 
desirable, it is considered undesirable for the exec utility, which is 
designed without the use of the standard library.  The tactic I chose to 
use in this patch was to filter the library out of the $LDFLAGS 
variable, but I wonder if a better approach would be alter the 
makefile.common file so that the library isn't included by default on 
the link line.

The second concern is a compile failure when using the Compaq compiler. 
  In particular, this compiler fails on code that was implemented as a 
workaround for STDCXX-291.  I feel that it is (slightly) more efficient 
to use the older code than the STDCXX-291 workaround code, so I chose to 
make that the default path, but it would also be possible to make the 
alternate code path specific to the Compaq compiler (via the __DECCXX macro)

--Andrew Black

Changelog:
	* GNUmakefile.bin (LDFLAGS.exec): Define LDFLAGS variant, filtering out 
the stdcxx library
	  (exec): use LDFLAGS.exec rather than LDFLAGS
	* exec.cpp (wait_for_child): Use workaround for STDCXX-291 only with HP 
aCC (causes failure with Compaq CXX)

Re: [PATCH]Assorted cleanup

[Martin Sebor <se...@roguewave.com>]

Andrew Black wrote:
[...]
> This is a somewhat cosmetic change, but it does allow the exec utility 
> to run if the library were to build incorrectly such that it crashes 
> during initialization.
> 
> exec_unlink.diff Changelog:
>     * GNUmakefile.bin (LDFLAGS.exec): Define LDFLAGS variant, filtering 
> out the stdcxx library
>       (exec): use LDFLAGS.exec rather than LDFLAGS

Okay. Please do try to capitalize/terminate your sentences in
the ChangeLog. ChangeLogs deserve the same attention to detail
as the code changes they document.

> 
[...]
> I still feel that it is more efficient to assign the signal handler 
> reference to the element in the sigaction structure rather than using 
> memcpy, but I also agree that the use of the typedef is cleaner than 
> using the #ifndef logic.

Directly assigning the pointer probably is going to be more
efficient in most cases (some optimizers, such as gcc, manage
to eliminate the memcpy call and replace it with the equivalent
all over of the plain assignment, but others, such as Sun C++,
do not). But the point of the memcpy call here is to avoid the
incompatibility and to work around the HP aCC bug. We don't
need to be that concerned with efficiency in this code, so I
wouldn't lose sleep over it if I were you :)

> 
> compaqfix.diff Changelog:
>     * exec.cpp (alarm_handler) [!_WIN32 && !_WIN64]:

There is no need for the &&. .. part here or in code as the _WIN32
macro is guaranteed to be defined. Please stop using it -- it only
makes the text harder to read. Other than that, if this compiles
with aCC 6 and Compaq C++ 7 go ahead and commit it.

Thanks
Martin

  Define typedef with
> signature matching that of handle_alrm.
>       (wait_for_child) [!_WIN32 && !_WIN64]: Use alarm_handler typedef 
> for type of local variable storing reference to handle_alrm.
> 
> --Andrew Black
> 
> 
> ------------------------------------------------------------------------
> 
> Index: etc/config/GNUmakefile.bin
> ===================================================================
> --- etc/config/GNUmakefile.bin	(revision 472548)
> +++ etc/config/GNUmakefile.bin	(working copy)
> @@ -45,6 +45,9 @@
>    LDFLAGS += $(CPPFLAGS)
>  endif   # ($(CXX_REPOSITORY),)
>  
> +# Don't want to link exec utility with stdlib, so create our own LDFLAGS var
> +LDFLAGS.exec = $(filter-out -l$(LIBBASE),$(LDFLAGS))
> +
>  ##############################################################################
>  #  TARGETS
>  ##############################################################################
> @@ -55,8 +58,8 @@
>  
>  # link the run utility
>  exec: runall.o cmdopt.o output.o util.o exec.o display.o
> -	@echo "$(LD) $^ -o $@ $(LDFLAGS) $(LDLIBS)" >> $(LOGFILE)
> -	$(LD) $^ -o $@ $(LDFLAGS) $(LDLIBS) $(TEEOPTS)
> +	@echo "$(LD) $^ -o $@ $(LDFLAGS.exec) $(LDLIBS)" >> $(LOGFILE)
> +	$(LD) $^ -o $@ $(LDFLAGS.exec) $(LDLIBS) $(TEEOPTS)
>  
>  # link the localedef utility
>  localedef: localedef.o aliases.o charmap.o codecvt.o collate.o ctype.o \
> 
> 
> ------------------------------------------------------------------------
> 
> Index: exec.cpp
> ===================================================================
> --- exec.cpp	(revision 472548)
> +++ exec.cpp	(working copy)
> @@ -360,6 +360,8 @@
>          alarm_timeout = 1;
>  }
>  
> +typedef void (*alarm_handler)(int);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> @@ -421,7 +423,7 @@
>      /* avoid extern "C"/"C++" mismatch due to an HP aCC 6 bug
>         (see STDCXX-291)
>      */
> -    void (*phandler)(int) = handle_alrm;
> +    alarm_handler phandler = handle_alrm;
>      memcpy (&act.sa_handler, &phandler, sizeof act.sa_handler);
>  
>      sigaction (SIGALRM, &act, 0);

Re: [PATCH]Assorted cleanup

[Andrew Black <ab...@roguewave.com>]

Martin Sebor wrote:
> Andrew Black wrote:
>> Greetings all.
>>
>> Attached is a patch that aims to resolve a couple minor compile issues 
>> with the exec utility.
> 
> The two patches should probably go in separately since they are
> completely unrelated.

Patches split and attached

> 
>>
>> The first concern is the current link behavior, where the standard 
>> library is linked into all executables.  While this is normally 
>> desirable, it is considered undesirable for the exec utility, which is 
>> designed without the use of the standard library.  The tactic I chose 
>> to use in this patch was to filter the library out of the $LDFLAGS 
>> variable, but I wonder if a better approach would be alter the 
>> makefile.common file so that the library isn't included by default on 
>> the link line.
> 
> The ideal approach that we agreed on some time ago was to build
> the exec utility as a C program. But yours seems like a good enough
> workaround in the meantime (I assume the patch does in fact work
> around an actual problem and isn't just cosmetic).

This is a somewhat cosmetic change, but it does allow the exec utility 
to run if the library were to build incorrectly such that it crashes 
during initialization.

exec_unlink.diff Changelog:
     * GNUmakefile.bin (LDFLAGS.exec): Define LDFLAGS variant, filtering 
out the stdcxx library
       (exec): use LDFLAGS.exec rather than LDFLAGS

> 
>>
>> The second concern is a compile failure when using the Compaq 
>> compiler.  In particular, this compiler fails on code that was 
>> implemented as a workaround for STDCXX-291.  I feel that it is 
>> (slightly) more efficient to use the older code than the STDCXX-291 
>> workaround code, so I chose to make that the default path, but it 
>> would also be possible to make the alternate code path specific to the 
>> Compaq compiler (via the __DECCXX macro)
> 
> I was going to look into it but you beat me to it. The problem
> is that the caller is a C++ function and so the type of the local
> function pointer variable is one to a extern "C++" function. We
> can't initialize it with a "C" function. What we should do here
> is declare a typedef for handle_alrm in the same extern "C" block
> as the handler itself and then use the typedef to declare the
> local function pointer.

I still feel that it is more efficient to assign the signal handler 
reference to the element in the sigaction structure rather than using 
memcpy, but I also agree that the use of the typedef is cleaner than 
using the #ifndef logic.

compaqfix.diff Changelog:
     * exec.cpp (alarm_handler) [!_WIN32 && !_WIN64]: Define typedef 
with signature matching that of handle_alrm.
       (wait_for_child) [!_WIN32 && !_WIN64]: Use alarm_handler typedef 
for type of local variable storing reference to handle_alrm.

--Andrew Black

Re: [PATCH]Assorted cleanup

[Martin Sebor <se...@roguewave.com>]

Andrew Black wrote:
> Greetings all.
> 
> Attached is a patch that aims to resolve a couple minor compile issues 
> with the exec utility.

The two patches should probably go in separately since they are
completely unrelated.

> 
> The first concern is the current link behavior, where the standard 
> library is linked into all executables.  While this is normally 
> desirable, it is considered undesirable for the exec utility, which is 
> designed without the use of the standard library.  The tactic I chose to 
> use in this patch was to filter the library out of the $LDFLAGS 
> variable, but I wonder if a better approach would be alter the 
> makefile.common file so that the library isn't included by default on 
> the link line.

The ideal approach that we agreed on some time ago was to build
the exec utility as a C program. But yours seems like a good enough
workaround in the meantime (I assume the patch does in fact work
around an actual problem and isn't just cosmetic).

> 
> The second concern is a compile failure when using the Compaq compiler. 
>  In particular, this compiler fails on code that was implemented as a 
> workaround for STDCXX-291.  I feel that it is (slightly) more efficient 
> to use the older code than the STDCXX-291 workaround code, so I chose to 
> make that the default path, but it would also be possible to make the 
> alternate code path specific to the Compaq compiler (via the __DECCXX 
> macro)

I was going to look into it but you beat me to it. The problem
is that the caller is a C++ function and so the type of the local
function pointer variable is one to a extern "C++" function. We
can't initialize it with a "C" function. What we should do here
is declare a typedef for handle_alrm in the same extern "C" block
as the handler itself and then use the typedef to declare the
local function pointer.

Martin

> 
> --Andrew Black
> 
> Changelog:
>     * GNUmakefile.bin (LDFLAGS.exec): Define LDFLAGS variant, filtering 
> out the stdcxx library
>       (exec): use LDFLAGS.exec rather than LDFLAGS
>     * exec.cpp (wait_for_child): Use workaround for STDCXX-291 only with 
> HP aCC (causes failure with Compaq CXX)
> 
> 
> ------------------------------------------------------------------------
> 
> Index: etc/config/GNUmakefile.bin
> ===================================================================
> --- etc/config/GNUmakefile.bin	(revision 472548)
> +++ etc/config/GNUmakefile.bin	(working copy)
> @@ -45,6 +45,9 @@
>    LDFLAGS += $(CPPFLAGS)
>  endif   # ($(CXX_REPOSITORY),)
>  
> +# Don't want to link exec utility with stdlib, so create our own LDFLAGS var
> +LDFLAGS.exec = $(filter-out -l$(LIBBASE),$(LDFLAGS))
> +
>  ##############################################################################
>  #  TARGETS
>  ##############################################################################
> @@ -55,8 +58,8 @@
>  
>  # link the run utility
>  exec: runall.o cmdopt.o output.o util.o exec.o display.o
> -	@echo "$(LD) $^ -o $@ $(LDFLAGS) $(LDLIBS)" >> $(LOGFILE)
> -	$(LD) $^ -o $@ $(LDFLAGS) $(LDLIBS) $(TEEOPTS)
> +	@echo "$(LD) $^ -o $@ $(LDFLAGS.exec) $(LDLIBS)" >> $(LOGFILE)
> +	$(LD) $^ -o $@ $(LDFLAGS.exec) $(LDLIBS) $(TEEOPTS)
>  
>  # link the localedef utility
>  localedef: localedef.o aliases.o charmap.o codecvt.o collate.o ctype.o \
> Index: util/exec.cpp
> ===================================================================
> --- util/exec.cpp	(revision 472548)
> +++ util/exec.cpp	(working copy)
> @@ -421,8 +421,12 @@
>      /* avoid extern "C"/"C++" mismatch due to an HP aCC 6 bug
>         (see STDCXX-291)
>      */
> +#ifndef __HP_aCC
> +    act.sa_handler = handle_alrm;
> +#else
>      void (*phandler)(int) = handle_alrm;
>      memcpy (&act.sa_handler, &phandler, sizeof act.sa_handler);
> +#endif
>  
>      sigaction (SIGALRM, &act, 0);
>