[email protected] fails to catch an exception #850
Comments
|
@rinigus that should have exactly nothing to do with what you call a "Related PR" unless the old code in question was broken such that Scrolling the error message top-to-bottom and left-to-right with all unrelated lines is incredibly tedious and frustrating, so I'm cutting it down to the bare minimum that is important: What, strangely, seems to happen is that Alternatively AOSP could be compiling our HAL without RTTI (https://stackoverflow.com/questions/74441543/why-stdinvalid-argument-is-not-caught-with-no-rtti-in-macos-m1-environment)? Again nothing that the "Related PR" changed? |
|
I see it for the first time. |
|
@rinigus can you provide me your |
|
@MarijnS95 : I will check PR closer and will trim logs more next time :) . Dropped "Related PR" from original message. @bartcubbins: let me bake a new one and check that it fails the same way |
|
@bartcubbins despite the broken log messages in @rinigus said he came from a fresh stock, so it's quite possible that they have the file there in a different format; unfortunately the Also note that |
|
Very strange, I don't have that issue anymore. Looks like after successful boot by AOSP and shutdown, it doesn't appear. It fails on first boot after you come from stock as in: That's the first successful boot after coming from stock 64.2.A.2.185 and after applying the patch from my original message. Before that was the boot where I was stuck on on "android" and exception issue as in the original message. After the first successful boot, I get logs similar to @bartcubbins. Although, have lots of errors related to selinux, but that's another general issue. @MarijnS95 - cross-posting, was looking for binary and I agree with you. So, it is hard to reproduce. We need to probably go back to stock and then return. However, issue with exception catching is still there. @MarijnS95 - I wonder how to check your idea regarding different compilation? @bartcubbins: File attached |
|
@rinigus without fixing this, I'd love to learn more about the downstream format. The entire goal of this HAL is to "persist" these states, but that's all thrown out of the window if we restart the counter from stock, and stock restarts it when coming back from SODP. Don't think I understand what you mean exactly with |
|
Re stock / aosp restart. Agree, if we can figure out the format. But I am not sure it is important in practice for many. I would expect majority of users sticking to one of the camps. For SFOS users, they get a device, flash it, and use on SFOS without going to stock. In my eyes, transition issues between stock and AOSP are minor / low priority. Right now, on nagara, basically nothing works and there are way bigger issues to fix :) Re cross-posting: wrote my message part regarding coming from fresh stock, started looking for file, and then saw your message. |
Yes, I am familiar with the code |
and |
|
What is weird that it manages to pass |
|
Did/does the file exist for you, @rinigus (if you come directly from stock)? If the file didn't exist that function shouldn't have passed (unless it returned an empty string in this case?). |
|
In short, I cannot trigger it when testing today. I flashed back stock, booted into it for setup and once more to confirm it was OK. As its not rooted, couldn't see any vendor/persist. Then flashed new AOSP14 images with the following void CycleCountBackupRestore::Read(const std::string &path, int &cycles) {
std::string buffer;
LOG(WARNING) << "Test Access: " << access(path.c_str(), F_OK);
if (!android::base::ReadFileToString(path, &buffer)) {
LOG(WARNING) << "Failed to read battery cycles from " << path;
return;
}
LOG(WARNING) << "Test Buffer: '" << buffer << "'";
buffer = ::android::base::Trim(buffer);
try {
cycles = std::stoi(buffer);
} catch (std::out_of_range &e) {
LOG(WARNING) << "Battery cycle count in persist storage file is out of bounds: " << path;
return;
} catch (std::invalid_argument &e) {
LOG(WARNING) << "Data format is wrong in persist storage file: " << path;
return;
} catch (...) {
LOG(WARNING) << "Data format is wrong in persist storage file and exception sneaked through: " << path;
LOG(WARNING) << "Failed Buffer: '" << buffer << "'";
LOG(WARNING) << "Failed Access: " << access(path.c_str(), F_OK);
return;
}
LOG(VERBOSE) << "Read " << cycles << " battery cycles from " << path;
}Corresponding logcat grep: So, here it is nicely caught before proceeding to But coming back to the question: should we replace it with |
|
I have added the following snippet into the int testi;
try {
testi = std::stoi("hello");
} catch (std::out_of_range &e) {
LOG(WARNING) << "Test std::out_of_range";
} catch (std::invalid_argument &e) {
LOG(WARNING) << "Test std::invalid_argument";
} catch (...) {
LOG(WARNING) << "Test all catch";
}and have in the logs: i.e. the original issue is still there - we don't catch the corresponding exception. |
|
The bigger issue is that the cycles file is never made as we have permission denied errors: And similar permission denied errors later in the log for "Write max battery capacity" |
|
You shouldn't be able to create a file in this location in sysfs, it's an attribute of the battery driver/device. |
Please use |
|
Sure. Will prepare PR with it. |
|
Weird, for some reason this method (https://github.com/sonyxperiadev/device-sony-common/blob/master/hardware/health/CycleCountBackupRestore.cpp#L78) is not called anymore. Is it normal? Don't want to start flashing stock and getting back to AOSP from it just to test it. I have added few log calls to test, but never got anything from it in logcat. |
|
The thing is... DSP charger firmware never increments cycle_count on Nagara, Yodo... and most likely on other devices with QTI Glink battery charger |
|
Does it mean that this method is not even called on a regular boot? Since that what it looks like for me right now and I cannot figure out why. |
|
I can clearly see that the method is called and prints a warning which is acceptable. Please provide me with the full logcat. I'm interested to understand what's going on there |
|
Thank you very much! Will be able to provide it tonight. |
|
Please find attached full logcat. That's using current CycleCountBackupRestore::Read as it is in common repo (no patches). Few changes in my tree:
|
|
OK, on the last boot I can see the debug messages again from CycleCountBackupRestore::Read. Will work on PR for this. |
I converted our Health HAL to AIDL and now I catch the proper invalid_argument exception |
|
Excellent! I think it is way better than my submitted PR, thank you very much. Is this conversion to AIDL already merged? |
|
Not yet, it's in my private branch. I have a lot of different changes here besides this one that need to be sorted out |
It is hard to believe that converting from a HIDL to AIDL HAL has an effect on how exceptions are handled in C++ which should remain mostly unchanged. Unless compilation flags for our own @bartcubbins just as a sanity-check, have you made sure that the exception isn't caught for you in the previous HIDL variant, and it isn't some configuration/environment "issue" / mismatch on Rinigus' side? |
|
I'm 1000% sure. Just checked again. I have the same behavior as @rinigus |
|
Thanks for confirming, it is still very confusing and unexpected 😅. Looking forward to seeing your changes to i.e. |
Platform: nagara
Device: pdx223
Kernel version: 5.15
Android version: android-14_r67
Software binaries version: Patched SW_binaries_for_Xperia_Android_13_5.15_v4a_nagara.zip
Previously working on
Same issue was for me on AOSP13 as well, recent builds.
Description
On boot, device is stuck on ANDROID logo. In logcat, there are failures with [email protected]:
Symptoms
Stuck on Android logo while booting
How to reproduce
New build
Additional context
Maybe important: last stock was
XQ-CT54_Customized EU-UK_64.2.A.2.185While maybe it cannot reach battery data, the code seems to have corresponding exception handling in https://github.com/sonyxperiadev/device-sony-common/blob/master/hardware/health/CycleCountBackupRestore.cpp#L92
I could make it work by adding
But I cannot figure out why the original exception does not catch it.
cc: @MarijnS95
The text was updated successfully, but these errors were encountered: