Page MenuHomePhabricator

Allow -gsplit-dwarf for all ELF targets, not just Linux
AcceptedPublic

Authored by emaste on Mar 20 2015, 9:06 AM.

Details

Reviewers
echristo

Diff Detail

Event Timeline

emaste updated this revision to Diff 22353.Mar 20 2015, 9:06 AM
emaste retitled this revision from to Allow -gsplit-dwarf for all ELF targets, not just Linux.
emaste updated this object.
emaste edited the test plan for this revision. (Show Details)
emaste added a reviewer: echristo.
emaste added a subscriber: Unknown Object (MLST).
emaste updated this revision to Diff 22364.Mar 20 2015, 12:02 PM

Add a test case

There was talk about a diagnostic for targets that don't support split dwarf. Did you decide not to do that?

Sounds like just turning it on for everyone & letting objcopy print a
failure that it doesn't support the flag would be reasonable? No need for a
particular error from clang. But I dunno.

echristo edited edge metadata.Mar 20 2015, 2:17 PM

Agreed.

objcopy's default behavior on seeing an unrecognized switch is to dump its entire help text.
That's not what I'd call user-friendly.

Please disable this for PS4? Our toolchain isn't ready to support this.

Please disable this for PS4? Our toolchain isn't ready to support this.

Presumably you wouldn't pass -gsplit-dwarf to clang when building for PS4 though?

Please disable this for PS4? Our toolchain isn't ready to support this.

Presumably you wouldn't pass -gsplit-dwarf to clang when building for PS4 though?

*I* wouldn't, but licensees love to try stuff out even if we don't claim it
works. Currently, adding -gsplit-dwarf to a command line silently has
no effect. With your patch, I get:

clang.exe: error: unable to execute command: program not executable

which will inevitably cause a support call and cost us time and effort.
I'd prefer to preserve the "silently has no effect" behavior, at least until
we are able to support this properly.

30 years of delivering enterprise software tells me this:
If a switch you don't claim to support does nothing, nobody will complain. (Unless they're looking for an excuse to jerk your chain.)
If a switch you don't claim to support says "sorry, that switch is not supported" nobody will complain, period.
If a switch you don't claim to support says "program not executable" you WILL hear about it.

I'd be happiest with the second case, and fine with the first case. The third case is unacceptable.
--paulr

From: cfe-commits-bounces@cs.uiuc.edu [mailto:cfe-commits-bounces@cs.uiuc.edu] On Behalf Of David Blaikie
Sent: Saturday, March 21, 2015 2:25 PM
To: reviews+D8484+public+374fa8dd1089857c@reviews.llvm.org
Cc: llvm cfe
Subject: Re: [PATCH] Allow -gsplit-dwarf for all ELF targets, not just Linux

emaste updated this revision to Diff 22469.Mar 23 2015, 8:01 AM
emaste edited edge metadata.

Enable -gsplit-dwarf explicitly for FreeBSD as well - no consensus for the previous patch enabling for all ELF targets at present.

WFM. if the condition gets any more complicated it should be factored out into a helper function.

echristo accepted this revision.Mar 23 2015, 9:53 AM
echristo edited edge metadata.

Sure, have at.

-eric

This revision is now accepted and ready to land.Mar 23 2015, 9:53 AM
joerg added a subscriber: joerg.Mar 23 2015, 4:56 PM

Well, in this case I would prefer to black list systems that want to ignore the option. I think that's a far better approach. While I don't agree with the reasoning of the PS4 guys, I can understood the pain. But it should be applied to other systems as it violates POLA.

In D8484#145724, @joerg wrote:

Well, in this case I would prefer to black list systems that want to ignore the option. I think that's a far better approach.

A black list that excluded PS4 or a white list that didn't include PS4 would be equally fine.
Emitting the "not supported for this target" diagnostic would be gravy.

While I don't agree with the reasoning of the PS4 guys, I can understood the pain. But it should be applied to other systems as it violates POLA.

Not clear what "it" you are referring to here. But a diagnostic that says "program not executable" seems more astonishing than either of the alternatives.

joerg added a comment.Mar 24 2015, 5:54 AM

We could add a toolchain flag whether objcopy is supposed to exist. PS4 could set that to false, everything else to true. That would allow at least a proper error message for the PS4 case and whoever else cares. I think silently failing here is very bad as the option changes the behavior of clang (it is supposed to create another output file after all).