On Fri, 11 Oct 2024 18:05:58 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> Matthias Baesken has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'origin/master' into JDK-8340801 >> - move ub.h to a better location >> - remove ubsan changes from jni_md.h >> - JDK-8340801 > > Not a review, just commenting. > > I very much support the work @MBaesken has done with ubsan in HotSpot. > There's a bit of a chicken or egg problem with this kind of thing. ubsan > isn't really usable and in any way supported or supportable until the code has > been made pretty much ubsan-clean. Matthias has made substantial progress > toward that for HotSpot, and I thank him for this effort. > > There are currently four uses of the disabling attribute in HotSpot. Two are > for intentional things (writing to address zero intentionally, for testing > signal handling and the like). One is for a known issue that is being worked > on, with an associated JBS issue; the attribute is being used to reduce > testing noise in the meantime. The fourth is related to some of my recent > work (adjacent to, not caused by), and something that I think ought to be > fixed. I'll be filing a JBS issue. Along the way there have been a > substantial number of real bugs uncovered and addressed. > > My only complaint has been a tendency to be a bit too quick to reach for the > disabling attribute, without sufficient analysis and attempt to resolve in a > way that corrects the problem. Of the issues I've observed, the result of a > real fix nearly always ben a straight-up improvement. I agree with @kimbarrett in his fear that a convenience macro may lower the hurdle of just suppressing an ubsan complaint. On the other hand, using a macro makes the code look cleaner. Should there ever be a need to change how ubsan is quiesced, it can be done at one place with the use of a macro. I therefore will give the change a LGTM. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21184#issuecomment-2410653806
