8320931: [REDO] dsymutil command leaves around temporary directories

[jaikiran]

Can I please get a review of this change will attempts to workaround an issue in dsymutil?

The previous attempt to use --reproducer Off has shown that it fails to build on some other Xcode versions other than 14.3.1. Users have reported it to fail on Xcode 15.0.1 and Xcode from a 12.4 devkit. The --reproducer option isn't being recognized on those versions.

This current PR proposes to use an alternate workaround, one which just sets an environment variable that Xcode 14.3.1 recognizes and prevents the creation of the leftover dsymutil-* temporary directories. Since this new approach uses an environment variable to workaround the issue, the command itself shouldn't ideally run into any usage issues. Once we agree upon this change, before integrating, I'll ask for inputs from those who had run into build issues to verify this alternate approach doesn't cause problems.

I have locally verified that this new approach continues to work and doesn't create those temporary directories. Additionally tier1, tier2, tier3 has completed successfully in our CI environment with this change.

Magnus, Erik, I looked around to see if there's any convention in setting such environment variables that will be used when invoking external tools during the build. I didn't find any similar usage. Although the current change in this PR is working, if there's a different and better way to do this, please do let me know.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8320931: [REDO] dsymutil command leaves around temporary directories (Bug - P4)

Reviewers

• Magnus Ihse Bursie (@magicus - Reviewer)
• Christoph Langer (@RealCLanger - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16876/head:pull/16876
$ git checkout pull/16876

Update a local copy of the PR:
$ git checkout pull/16876
$ git pull https://git.openjdk.org/jdk.git pull/16876/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16876

View PR using the GUI difftool:
$ git pr show -t 16876

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16876.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@jaikiran The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (0dc877eb)
• 00: Full (6ba87593)

[magicus]

Hm, I'm not entirely sure about this. It looks quite ugly, and we'll be stuck with it like forever. :-(

First of all, I think you should add the env var prefix to the DSYMUTIL variable itself, in configure. This is not only to make the makefile less ugly, but since you'd want the same workaround if we were to execute dsymutil elsewhere. (I actually missed this in the previous version of the fix.)

Then, I think it might be worth adding a version check of dsymutil, before prepending this variable, so that we can eventually get rid of this.

[jaikiran]

Hello Magnus, thank you for those inputs. Based on that I've now modified this PR to check if the dsymutil supports the --reproducer option and only if it supports it, I then use that option.

I started off with using the DSYMUTIL_REPRODUCER_PATH environment variable usage and thought of checking the dsymutil version to decide whether or not to set that environment variable. However, it's extremely unclear which exact version of dsymutil should be considered as supporting it. In the upstream llvm project, the commit that introduced this auto generation of temporary directories (along with the --reproducer option) is this one llvm/llvm-project@33b6891 and going by the git tags associated with that commit, it would appear that even 15.0.x version of dsymutil would support the --reproducer option. However, we have had reports that --reproducer isn't supported on:

Apple LLVM version 15.0.0
  Optimized build.
  Default target: arm64-apple-darwin23.1.0
  Host CPU: apple-m1

I think we can't rely on the version numbers of dsymutil alone to decide whether or not to set the DSYMUTIL_REPRODUCER_PATH environment variable.

So I decided to add a check in the build configuration to see if --reproducer option is supported by dsymutil and since we were checking for this option anyway, then I felt that when this option is supported, it makes sense to set --reproducer Off instead of the DSYMUTIL_REPRODUCER_PATH environment variable.

I have tested this change locally with:

dsymutil --version
Apple LLVM version 14.0.0 (clang-1400.0.29.202)
  Optimized build.
  Default target: arm64-apple-darwin23.1.0
  Host CPU: apple-a12

which doesn't support this option and with:

Apple LLVM version 14.0.3 (clang-1403.0.22.14.1)
  Optimized build.
  Default target: arm64-apple-darwin23.1.0
  Host CPU: apple-m1

which supports this option. I've verified that the build change correctly sets the --reproducer Off when this option is supported and the temporary directories don't get created.

tier1, tier2 and tier3 testing with this change is currently in progress.

[magicus]

I think it looks good. It would be great if we could have this tested by the people who ran into issues with the original implementation. I don't know who they were, though. I'll see if I can dig through the archives and find some to ping.

[openjdk]

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8320931: [REDO] dsymutil command leaves around temporary directories

Reviewed-by: ihse, clanger

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 32 new commits pushed to the master branch:

• 8f1d40b: 8321114: Rename "Unnamed Classes" to "Implicitly Declared Classes" better
• ecd335d: 8321107: Add more test cases for JDK-8319372
• 2476be4: 8320716: ResolvedModule::reads includes self when configuration contains two or more automatic modules
• 4ba94ef: 8320145: Compiler should accept final variable in Record Pattern
• 02ffab1: 8321141: VM build issue on MacOS after JDK-8267532
• 56d4c33: 8227529: With malformed --app-image the error messages are awful
• 76fea80: 8320608: Many jtreg printing tests are missing the @printer keyword
• c4732c2: 8320530: has_resolved_ref_index flag not restored after resetting entry
• e96e191: 8318586: Explicitly handle upcall stub allocation failure
• 630bafd: 8320826: call allocate_shared_strings_array after all strings are interned
• ... and 22 more: https://git.openjdk.org/jdk/compare/940f67c1a62c6f9462266f3a108649aca114cffa...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[jaikiran]

@RealCLanger Hello Christoph, in https://bugs.openjdk.org/browse/JDK-8320863 you noted that you ran into a build issue with Xcode version 13.1. Would you be able to test this current proposed patch in this PR against that setup (and any other setups you might have access to) and see if it builds fine?

[erikj79]

I have verified that configuring with an older devkit works as intended, i.e. the --reproducer option is not added to DSYMUTIL.

[RealCLanger]

@RealCLanger Hello Christoph, in https://bugs.openjdk.org/browse/JDK-8320863 you noted that you ran into a build issue with Xcode version 13.1. Would you be able to test this current proposed patch in this PR against that setup (and any other setups you might have access to) and see if it builds fine?

I'm testing this currently. Will let you know the result tomorrow.

[RealCLanger]

Looks good, passes SAP's build setup.

[jaikiran]

Thank you Christoph for running the tests.

[magicus]

I have now tested this with Xcode 15.0 as well and confirm that it works. I think this means that you can safely integrate now.

[jaikiran]

Thank you Magnus and others for helping review and test this change.

[jaikiran]

/integrate

[openjdk]

Going to push as commit 6f7bb79.
Since your change was applied there have been 43 commits pushed to the master branch:

• 8be3e39: 8320129: "top" command during jtreg failure handler does not display CPU usage on OSX
• 2f299e4: 8321182: SourceExample.SOURCE_14 comment should refer to 'switch expressions' instead of 'text blocks'
• 3a09a05: 8313722: JFR: Avoid unnecessary calls to Events.from(Recording)
• 42af8ce: 8308614: Enabling JVMTI ClassLoad event slows down vthread creation by factor 10
• 1839433: 8320941: Discuss receiver type handling
• 92f7e51: 8312098: Update man page for javadoc
• f6be7fd: 8321059: Unneeded array assignments in MergeCollation and CompactByteArray
• d568562: 8320798: Console read line with zero out should zero out underlying buffer
• 3087e14: 8320807: [PPC64][ZGC] C1 generates wrong code for atomics
• 54957ac: 8312560: Annotation on Decomposed Record Component in Enhanced For Loop Fails Compilation
• ... and 33 more: https://git.openjdk.org/jdk/compare/940f67c1a62c6f9462266f3a108649aca114cffa...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jaikiran Pushed as commit 6f7bb79.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.