bugGNU Octave - Bugs: bug #56505, setting CXX should affect...

 
 

bug #56505: setting CXX should affect mkoctfile link stage as well

Submitter:  Mike Miller <mtmiller>
Submitted:  Sun 16 Jun 2019 06:07:39 PM UTC
   
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 18 Sep 2019 04:12:51 PM UTC, comment #8: 

Looks good, thanks!

Mike Miller <mtmiller>
Group Member
Wed 18 Sep 2019 04:01:40 AM UTC, comment #7: 

I don't think there's much need to record CXXLD in the build-env system.  I removed that.  I checked the whole kit and kaboodle in here: https://hg.savannah.gnu.org/hgweb/octave/rev/633f7a8347c8.

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Wed 18 Sep 2019 01:34:46 AM UTC, comment #6: 

Ok, one more attachment, this time I just eliminated the entire attempt at bringing a default value for CXXLD into mkoctfile. I believe the default value should be empty. If the user doesn't set CXXLD, then it should fall back to the value of CXX, which is either built-in or also comes from the environment.

I'm indifferent on whether CXXLD should be added to build-env, since it's not really a configure build-time variable, it's a convenience variable that aliases to CXX by default.

(file #47546)

Mike Miller <mtmiller>
Group Member
Wed 18 Sep 2019 01:07:39 AM UTC, comment #5: 

I looked at and tested what you have so far.

The biggest thing I notice is that CXXLD is an Automake variable, not an Autoconf variable, so referencing it in configure.ac will always evaluate to an empty string.

I think maybe we should eliminate CXXLD and MKOCTFILE_CXXLD entirely from configure.ac, and instead push the logic into mkoctfile.in.cc itself, that CXXLD should be set to the value of OCTAVE_CONF_MKOCTFILE_CXX by default.

I honestly have no idea about the NetBSD and SunOS cases, maybe we should just drop those overrides and wait for complaints.

Overall, now that I see how Automake initializes CXXLD normally, I think we should avoid adding that variable to configure, but I still think the name makes sense for mkoctfile to use for its own purposes.

I also saw you had a few lines with trailing whitespace.

I put my suggestions into an updated version of your patch file, attached, and testing now.

(file #47545)

Mike Miller <mtmiller>
Group Member
Sun 15 Sep 2019 12:43:48 AM UTC, comment #4: 

@Mike: Attached is a patch that attempts to implement just one variable, CXXLD, when referring to the linker.

It works for me, but someone else needs to review it since this tweaks a lot of musty parts of the build system that haven't seen a change in years.

There are two things I am uncertain about.  First, if CXXLD is not set in the environment during the build then MKOCTFILE_CXXLD is initialized to "".  Perhaps we should do the same thing as in mkoctfile.in.cc and set CXXLD to CXX if it is not defined.

The second thing is that for BSD and Solaris systems configure.ac unilaterally sets CXXLD to "ld".  I don't like overriding a user's given choice.

(file #47510)

Rik <rik5>
Group administrator
Tue 03 Sep 2019 08:27:06 PM UTC, comment #3: 

@Mike: I agree.  Three different variables with slightly different names to specify the linker is bizarro.  Marking as confirmed.

Rik <rik5>
Group administrator
Mon 17 Jun 2019 08:29:30 PM UTC, comment #2: 

That seems reasonable to me.  I don't remember now why we have these separate variables.  I also agree that if CXXLD is not explicitly set, then it should use the value of CXX.

John W. Eaton <jwe>
Group administrator
Mon 17 Jun 2019 06:42:25 PM UTC, comment #1: 

I'm leaning towards combining the three linker variables 'DL_LD', 'LD_CXX', and 'SH_LD'. They all seem to be some definition of a linker, and in the typical use case they should all be set to the same thing as 'CXX'. I think the new combined variable should be called 'CXXLD' for consistency with GNU Automake.

Then part 2 would be to add logic to mkoctfile, such that if the user environment sets 'CXX' but not 'CXXLD', the value of 'CXX' should be used for both the 'CXX' and 'CXXLD' internal variables.

Any objections to any of that in theory?

Mike Miller <mtmiller>
Group Member
Sun 16 Jun 2019 06:07:39 PM UTC, original submission:  

The mkoctfile program uses a separate non-standard internal variable for the name of the  linker program to call when building oct files, and a separate one for the name of the linker to use when building standalone programs.

I think that setting the CXX variable should also override the name of the linker. If we still want the user to be able to set both the compiler and linker independently, we should use a more standard variable name, either something like LD or CXXLD.

Example of current behavior


>> mkoctfile -v dummy.cc
g++ -c -fPIC -I… -pthread -fopenmp -g -O2 dummy.cc -o /tmp/oct-Cwseid.o
g++ -I… -pthread -fopenmp -g -O2 -shared -Wl,-Bsymbolic -o dummy.oct  /tmp/oct-Cwseid.o
>>
>> setenv CXX clang
>> mkoctfile -v dummy.cc
clang -c -fPIC -I… -pthread -fopenmp -g -O2 dummy.cc -o /tmp/oct-MCpC7P.o
g++ -I… -pthread -fopenmp -g -O2 -shared -Wl,-Bsymbolic -o dummy.oct  /tmp/oct-MCpC7P.o


It would be more user friendly if the second command shown, the link stage, also used clang by virtue of the user setting CXX. Currently the user has to know about the special Octave environment variable 'DL_LD'.

As far as I can tell, 'DL_LD' is always set to the same thing as 'CXX' when Octave is built. So is the 'LD_CXX' variable. These two linker variables look redundant to me and I think we could combine them into one.

Mike Miller <mtmiller>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #47546:  mkoctfile-v3.patch added by mtmiller (22KiB - text/x-patch)
file #47545:  mkoctfile-v2.patch added by mtmiller (22KiB - text/x-patch)
file #47510:  bug56505.cset added by rik5 (25KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-09-18 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2019-09-18 mtmiller Attached File- Added mkoctfile-v3.patch, #47546
    2019-09-18 mtmiller Attached File- Added mkoctfile-v2.patch, #47545
    2019-09-16 rik5 StatusConfirmed Patch Submitted
    2019-09-15 rik5 Attached File- Added bug56505.cset, #47510
    2019-09-03 rik5 StatusNone Confirmed
    2019-06-16 mtmiller Carbon-CopyRemoved 80942 -

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code