Repository navigation
Conversation
github-actions
Bot
added
tool
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables support for both ELF and PE/COFF AOT snapshot formats on Windows x64. It replaces architecture-based compile-time checks with runtime magic-number detection of the AOT library format in FlutterProjectBundle::LoadAotData, and introduces a --windows-aot-format build option to configure the target format. Feedback suggests explicitly including the Abseil header for absl::StatusOr to follow the "Include What You Use" style guide, and refactoring GetAotDataSourceType to accept const std::filesystem::path& to avoid redundant path conversions on Windows.
| #include <memory> | ||
| #include <optional> | ||
| #include <string_view> |
There was a problem hiding this comment.
According to the Google C++ Style Guide (referenced in the Flutter Repository Style Guide), you should directly include the headers for all symbols used in the file ("Include What You Use"). Since absl::StatusOr is used in this file, please explicitly include <third_party/abseil-cpp/absl/status/statusor.h>.
| #include <memory> | |
| #include <optional> | |
| #include <string_view> | |
| #include <memory> | |
| #include <optional> | |
| #include <string_view> | |
| #include "third_party/abseil-cpp/absl/status/statusor.h" |
References
- The Google C++ Style Guide (followed by Flutter C++ code) mandates 'Include What You Use' to avoid relying on transitive includes. (link)
There was a problem hiding this comment.
it's already there, gemini glitched
| absl::StatusOr<FlutterEngineAOTDataSourceType> GetAotDataSourceType( | ||
| const std::string& path) { | ||
| constexpr uint8_t kElfMagic[] = {0x7f, 'E', 'L', 'F'}; | ||
| constexpr uint8_t kPeMagic[] = {'M', 'Z'}; | ||
|
|
||
| std::unique_ptr<fml::FileMapping> mapping = | ||
| fml::FileMapping::CreateReadOnly(path); |
There was a problem hiding this comment.
To avoid unnecessary path conversions and potential issues with non-ASCII characters on Windows, GetAotDataSourceType should take const std::filesystem::path& instead of const std::string&. This allows us to pass aot_library_path_ directly and use the wide-string overload of fml::FileMapping::CreateReadOnly via path.wstring().
| absl::StatusOr<FlutterEngineAOTDataSourceType> GetAotDataSourceType( | |
| const std::string& path) { | |
| constexpr uint8_t kElfMagic[] = {0x7f, 'E', 'L', 'F'}; | |
| constexpr uint8_t kPeMagic[] = {'M', 'Z'}; | |
| std::unique_ptr<fml::FileMapping> mapping = | |
| fml::FileMapping::CreateReadOnly(path); | |
| absl::StatusOr<FlutterEngineAOTDataSourceType> GetAotDataSourceType( | |
| const std::filesystem::path& path) { | |
| constexpr uint8_t kElfMagic[] = {0x7f, 'E', 'L', 'F'}; | |
| constexpr uint8_t kPeMagic[] = {'M', 'Z'}; | |
| std::unique_ptr<fml::FileMapping> mapping = | |
| fml::FileMapping::CreateReadOnly(path.wstring()); |
There was a problem hiding this comment.
FileMapping::CreateReadOnly has no wide-string overload. path_string is UTF-8 and fml::OpenFile converts it with Utf8ToWideString, so non-ASCII paths work (checked with Cyrillic and CJK paths).
| absl::StatusOr<FlutterEngineAOTDataSourceType> type = | ||
| GetAotDataSourceType(path_string); |
There was a problem hiding this comment.
Pass aot_library_path_ directly to GetAotDataSourceType now that it accepts const std::filesystem::path&.
| absl::StatusOr<FlutterEngineAOTDataSourceType> type = | |
| GetAotDataSourceType(path_string); | |
| absl::StatusOr<FlutterEngineAOTDataSourceType> type = | |
| GetAotDataSourceType(aot_library_path_); |
Towards: #187594
PR adds optional
flutter build windows --windows-aot-format=elf, which brings back optional builds for Windows x64 AOT snapshot as ELF, as before #187594 (functionality for ELF was still present for Windows ARM and Linux). The engine picks the loader from the file's magic number. For apps that choose ELF, this avoids the following PE/COFF problems (discussion from pr 187594):app.sosize with--obfuscate --split-debug-info(counter app)bUa (Byb:21)can't be decoded.symbolsdecodes toinnerFrame (main.dart:21:22)withpackage:native_stack_tracesapp.sowhile the app runs (in-place updaters)app.solink.exeAlso fixes a crash when app.so is missing or invalid in a non-ASCII path: streaming aot_library_path_ into FML_LOG converts it to the ANSI code page. Both logs now use the UTF-8 path_string. Regressed in #174471, missed by #179528.
Pre-launch Checklist