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

[David Holmes]

Hi Erik,

On 31/03/2014 10:28 PM, Erik Helin wrote:
> Hi David,
>
> thanks for having a look! Please see my reply inline.

Thanks for the additional cleanup!

Looks good.

David


> 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
>>>>>
>>>