fx_ver::parse SIGABRTs on Mac with new toolset update

[agocke]

Happens on both x64 and ARM64

/* static */
bool fx_ver_t::parse(const pal::string_t& ver, fx_ver_t* fx_ver, bool parse_only_production)
{
    bool valid = parse_internal(ver, fx_ver, parse_only_production);
    -> SIGABRT  assert(!valid || fx_ver->as_str() == ver);
    return valid;
}

Might be allocator confusion.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

/* static */
bool fx_ver_t::parse(const pal::string_t& ver, fx_ver_t* fx_ver, bool parse_only_production)
{
    bool valid = parse_internal(ver, fx_ver, parse_only_production);
    -> SIGABRT  assert(!valid || fx_ver->as_str() == ver);
    return valid;
}

Might be allocator confusion.

Author:	agocke
Assignees:	-
Labels:	area-Host
Milestone:	-

[VSadov]

I wonder - are we using fx_ver_t::as_str() in any other place?
If the method has a bug, it would be triggering more asserts. Unless this is the only place.

In my experience allocator is typically right, but also I do not think this code is new.

[VSadov]

I tried commenting out fx_ver_t::as_str() declaration/definition and it caused bunch of compiler errors. We are using this in other places. It is weird that it upsets allocator in this particular case.

Also weird why it started asserting with toolset update. Did we change c++ compiler version, by any chance? (could be a compiler bug)

[agocke]

Yup, we are. This somehow looks unique to this location.

[agocke]

Also weird why it started asserting with toolset update. Did we change c++ compiler version, by any chance? (could be a compiler bug)

I don't think so. #67771 looks like it doesn't directly change with the native toolset. Can't completely rule it out though.

[VSadov]

fx_ver.cpp did not change for more than a year.
Something else must have changed - like OSX update maybe?

[agocke]

We're using the same machines and it's not failing in main, so it would have to be brought down by that PR. That PR does bring down a new SDK version, but I don't know how that could make a difference. I suppose it is interesting that the actual version number that's being passed to fx_ver::parse has changed.

[VSadov]

I have seen this on my machine when working on singlefile fix.
(I initially thought it is sideeffect of some corruption caused by the bug, so I commented assert out while investigating)

I do not think I have the PR. My baseline was relatively recent main, perhaps a few days old.

[am11]

stringstream is locale dependent and is slower than append: https://stackoverflow.com/a/19845594/863980. Maybe we should switch as_string to use pal::string_t append for general goodness and fix this issue in the process?

[agocke]

I have seen this on my machine when working on singlefile fix.

Oh, in that case it may definitely be a machine or toolchain thing. I'll try to get it to repro for main on x64.

[VSadov]

@am I do not think the use is very perf sensitive, but we could change if otherwise it is the same. I think we still need to understand the issue though. Just in case if we may have more cases.

[agocke]

I debugged through this and found stringstream::str() in fx_ver delegates to operator new in clrhost_dependencies, but deallocate goes to libc++abi.dyliboperator delete`. No idea why though

[janvorli]

@agocke we've had this kind of a problem multiple times in the past. It can be triggered e.g. when some code from the STL headers gets inlined and some comes from the libc++ library. The inlined code can pick our operator implementation. Might be due to our headers of utilcode being included in a bad order w.r.t. the STL ones or just being included by accident.