RFR: 8033251: Use DWARF debug symbols for Linux 32-bit as default

[Erik Helin]

Hi David,

thanks for having a look! Please see my reply inline.

On 2014-03-29 05:43, David Holmes wrote:
> On 29/03/2014 4:26 AM, Coleen Phillimore wrote:
>>
>> On 3/28/14 12:44 PM, Erik Helin wrote:
>>> Hi Coleen,
>>>
>>> thanks for having a look at the patch!
>>>
>>> On 2014-03-28 17:30, Coleen Phillimore wrote:
>>>>
>>>> I thought you'd be able to remove the DEBUG_BINARIES logic with this
>>>> change.  It appears there are no platforms left that use stabs with
>>>> gcc.
>>>
>>> Yeah, I also noticed that this change enables cleanups in the
>>> Makefiles. However, I think it is better to do these cleanups as a
>>> second change after this one, what do you think?
>>
>> A cleanup CR is fine.
>
> Unless the cleanup is coming "tomorrow" I disagree. What have now is
> comments that make no sense given the change that has been made. And as
> every defined platform seems to use -g we can get rid of all this
> per-platform logic and simply check for clang. All of which can be done
> without doing the DEBUG_BINARIES cleanup. Eg delete the per-platform
> flags and simply have, for example,
>
>      # Override per-platform if the default -g doesn't suite
>      OPT_CFLAGS += $(OPT_CFLAGS/$(BUILDARCH))
>      ifeq ($(OPT_CFLAGS/$(BUILDARCH)),)
>        ifeq ($(USE_CLANG), true)
>          # Clang doesn't understand -gstabs
>          OPT_CFLAGS += -g
>        else
>          OPT_CFLAGS += -gstabs
>        endif
>      endif

I wanted this change to only be about changing from STABS to DWARF, but 
based on your feedback I cleaned up some code in the makefile as well:

- Incremental webrev:
   http://cr.openjdk.java.net/~ehelin/8033251/webrev.00-01/
- New webrev:
   http://cr.openjdk.java.net/~ehelin/8033251/webrev.01/

The default is now to use DWARF for all platforms, but this can be 
changed by using CFLAGS, for example FASTDEBUG_CFLAGS/amd64=-gstabs will 
give you STABS debug symbols for a fastdebug build on 64-bit Linux. 
Having -g as default also enables us to get rid of all the $(USE_CLANG) 
checks.

The new patch also fixes a small bug where FASTDEBUG_CFLAGS was set to 
$(DEBUG_CFLAGS/$(BUILDARCH)), it should be set to 
$(FASTDEBUG_CFLAGS/$(BUILDARCH)).

Testing of new patch:
- JPRT
- Building builds for different targets with different debug symbols
   locally.

Thanks,
Erik

> In the future this flag should also potentially be coming in via spec.gmk.
>
> David
>
>>>
>>> On 2014-03-28 17:30, Coleen Phillimore wrote:
>>>> Does this change add -g for saproc, adlc and jsig compilations also?
>>> It seems like saproc uses SA_DEBUG_CFLAGS which only is being set if
>>> DEBUG_BINARIES=true. If DEBUG_BINARIES is not true, then it seems like
>>> we don't generate debug symbols (please correct me if I'm wrong).
>>>
>>> The above also seems to be the case for libjsig.so.
>>>
>>
>> These seem broken, but I don't know the makefiles well enough to comment.
>>
>> I think this change is good.  People have been asking for it for years
>> and now that the size problem with dwarf has been fixed, I'm glad you
>> are making this change.
>>
>> Thanks,
>> Coleen
>>> Looking in make/linux/makefiles/adlc.make it seems like adlc is using
>>> -g for GCC 3.3 or newer.
>>>
>>> Thanks,
>>> Erik
>>>
>>>> Coleen
>>>>
>>>> On 3/28/14 12:12 PM, Erik Helin wrote:
>>>>> Hi all,
>>>>>
>>>>> this patch changes the Linux Makefiles to use DWARF as the default
>>>>> debug symbol format for Linux 32-bit instead of STABS. The main
>>>>> arguments for using DWARF are:
>>>>>
>>>>>   - DWARF provides a superior debugging experience compared to STABS.
>>>>>   - STABS support in GCC/GDB is in maintenance mode [0].
>>>>>   - External tools (e.g. perf on Linux) often understands DWARF but
>>>>> not
>>>>>     STABS.
>>>>>   - Compilation on 32-bit Linux can make use of precompiled headers.
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~ehelin/8033251/webrev.00/
>>>>>
>>>>> Testing:
>>>>> - Compiling with GCC 4.3.4, GCC 4.7.3 and GCC 4.8.1
>>>>> - JPRT
>>>>>
>>>>> Thanks,
>>>>> Erik
>>>>>
>>>>> [0]: https://sourceware.org/ml/binutils/2013-01/msg00028.html
>>>>
>>