Ticket #2089 (closed bug: fixed)

Opened 3 years ago

Last modified 3 years ago

LLVM cflags prevent debug builds

Reported by: plobsing Owned by: jkeenan
Priority: normal Milestone:
Component: configure Version: 3.2.0
Severity: medium Keywords:
Cc: bacek Language:
Patch status: applied Platform:

Description

llvm cflags may contain '-DNDEBUG'. If this is added to parrot's cflags, it disables debug builds.

This configure step should

  • give users the option of using llvm or not
  • strip out options that alias parrot configure options

Since we aren't even linking to llvm, perhaps we should simply delete the configure step.

Attachments

llvm.no.ccflags.diff Download (0.6 KB) - added by jkeenan 3 years ago.
Don't add LLVM's ccflags to Parrot's

Change History

in reply to: ↑ description   Changed 3 years ago by jkeenan

  • cc bacek added

Replying to plobsing:

Since we aren't even linking to llvm, perhaps we should simply delete the configure step.

This is something I'm willing to consider. I originally added a configuration probe for LLVM back in 2009 upon request from whiteknight. At that time, auto::llvm followed LLVM documentation quite closely for the simple purpose of determining whether any version of LLVM was installed on the target box or not. We didn't link to it and, as you report, still don't.

In the last several months, however, bacek has been using LLVM in the opsc_llvm and jit_prototype branches. It turns out that simply determining whether a box has LLVM is insufficient for our purposes. A box must have LLVM 2.7 or newer. Neither of the two machines I have available can install a version that new. Hence, I have no way of seeing into what impact a positive result for LLVM has on things like cflags. (BTW, you can use Parrot::Configure::Trace to see how the recorded configuration values change from one step to the next.)

I copied the most recent version of config/auto/llvm.pm from theopsc_llvm into master because it seemed to be a better probe. But if we're not using it yet in master then we can delete it and only re-insert it when and if we understand all its side effects.

Recommend discussing this at #parrotsketch on Tuesday April 12. cc-ing bacek.

Thank you very much.

kid51

  Changed 3 years ago by jkeenan

TT #1075 was the ticket in which we added auto::llvm.

follow-up: ↓ 4   Changed 3 years ago by bacek

Hello.

I think we just shouldn't add llvm's cflags to parrot's cflags and keep them separate.

-- Bacek

Changed 3 years ago by jkeenan

Don't add LLVM's ccflags to Parrot's

in reply to: ↑ 3 ; follow-up: ↓ 6   Changed 3 years ago by jkeenan

Replying to bacek:

Hello. I think we just shouldn't add llvm's cflags to parrot's cflags and keep them separate.

plobsing, bacek:

Would the patch attached, llvm.no.ccflags.diff, suffice?

kid51

  Changed 3 years ago by bacek

+1 for patch.

in reply to: ↑ 4   Changed 3 years ago by plobsing

  • patch set to new

Replying to jkeenan:

Replying to bacek:

Hello. I think we just shouldn't add llvm's cflags to parrot's cflags and keep them separate.

plobsing, bacek: Would the patch attached, llvm.no.ccflags.diff, suffice? kid51

The attached patch addresses the current issue, should be sufficient to close this ticket, and I'm in favour of applying it.

However, I'd also like to see an explicit Configure.pl option to opt-out of LLVM, similar to what we do for other external libraries (eg: --without-libffi, --without-pcre, etc...).

  Changed 3 years ago by jkeenan

  • status changed from new to assigned
  • owner set to jkeenan

Created commit 568c380: [configure] Don't pick up LLVM's ccflags.

  Changed 3 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed
  • patch changed from new to applied
Note: See TracTickets for help on using tickets.