Tamarin's __DEBUGGING__ macro conflicts with Mac system headers

VERIFIED FIXED

Status

VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
(filing this bug against Tamarin VM, but it's really against the Tamarin build system)

The Tamarin build system defines __DEBUGGING__ via a command-line option to the compiler.  It doesn't appear to be used anywhere, though.

We should remove it; there's a Mac OS X system header, CarbonCore/Debugging.h, which contains this code:

  #ifndef __DEBUGGING__
  #define __DEBUGGING__
  ...
  #endif /* __DEBUGGING__ */

If __DEBUGGING__ is defined on the command line, you basically can't include this header.  This actually tripped me up doing build integration for ActionMonkey.
(Assignee)

Comment 1

11 years ago
Created attachment 279104 [details] [diff] [review]
remove __DEBUGGING__ from both the cross-platform build system and the Mac xcode project files
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 2

11 years ago
It appears __DEBUGGING__ was added intentionally to disable that header, due to a naming conflict between pcre and Carbon.  So the attached patch won't work.  Maybe we can fix this some other way.
(Assignee)

Comment 3

11 years ago
This conflict between pcre and Carbon is fixed in the latest release of pcre, version 7.3.  We're using version 6.0.  Any reason not to upgrade?

Comment 4

11 years ago
Not that I can think of.

Comment 5

11 years ago
Jason, the PCRE in Tamarin is a modified version to make it compatible with ECMAScript. Here's a note from Chris Nuuja who made the changes to the Tamarin PCRE:

I've made lots of small mods and one or two significant mods.  ECMAScript RegExp differs from the pcre standard in a few significant ways.  There are lots of expressions which will evaluate differently between the two.  

If the new changes don't affect the code I moded, then it should be simple.  Look at the change history for pcre_compile.cpp and pcre_exec.cpp  (look in mainline flashPlayer/avmplus/pcre.  For some reason /as/avmplus/pcre is missing a lot of change history).
(Assignee)

Comment 6

11 years ago
In that case-- Since it is a modified version of PCRE anwyay, there's no harm manually patching in the few lines I need.

+It turns out that the Mac Debugging.h header also defines the macro DPRINTF, so
+be absolutely sure we get our version. */
+
+#undef DPRINTF

I'll post a new patch with that change.  (If you want to upgrade to PCRE 7.3 anyway, let me know.)

Comment 7

11 years ago
Sounds like we should submit our changes (with suitable ifdef's) back to the PCRE maintainers, so we can track the current version and not have to re-make them...
(Assignee)

Comment 8

11 years ago
Created attachment 279114 [details] [diff] [review]
fix conflict between pcre and Carbon headers

Here's the fix.  (Apply both patches.)

For some reason Bugzilla is not showing me controls for requesting a code review.  So, this is me requesting a review.  :-)  Anyone?
(Assignee)

Updated

11 years ago
Attachment #279104 - Flags: review?(dansmith)
(Assignee)

Updated

11 years ago
Attachment #279114 - Flags: review?(dansmith)

Updated

11 years ago
Attachment #279114 - Flags: review?(dansmith) → review+

Updated

11 years ago
Attachment #279104 - Flags: review?(dansmith) → review+
(Assignee)

Comment 9

11 years ago
pushed changesets d81eb3457771 and e80d62e610e4.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.