Supporting custom section from config.json#191
Supporting custom section from config.json#191HimanshuChoudhary-Xilinx wants to merge 6 commits intoXilinx:main-gefrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for custom sections from config.json at three hierarchical levels: global, kernel, and instance. Custom sections are defined in JSON configuration files and loaded from external files, allowing additional data to be embedded in the final ELF output.
Key Changes:
- Added parsing logic for "customer_section" arrays from JSON at global, kernel, and instance levels
- Implemented data structures to track custom sections throughout the preprocessing and encoding pipeline
- Integrated custom section writers into the ELF generation process
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cpp/preprocessor/preprocessor_input.h | Added custom section storage and accessor methods to base preprocessor input class |
| src/cpp/preprocessor/aie2ps/aie2ps_preprocessor_input.h | Implemented custom section parsing and storage at all three levels for AIE2PS |
| src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h | Propagated custom sections from input to output during preprocessing |
| src/cpp/preprocessor/aie2ps/aie2ps_preprocessed_output.h | Added custom section storage and accessors to preprocessed output classes |
| src/cpp/preprocessor/aie2/aie2_blob_preprocessor_input.h | Added custom section data structures and parsing helper for AIE2 |
| src/cpp/preprocessor/aie2/aie2_blob_preprocessor_input.cpp | Implemented custom section parsing from JSON for AIE2 at all levels |
| src/cpp/preprocessor/aie2/aie2_blob_preprocessor.h | Propagated custom sections from input to output in AIE2 preprocessor |
| src/cpp/preprocessor/aie2/aie2_blob_preprocessed_output.h | Added custom section storage structures to AIE2 output classes |
| src/cpp/encoder/aie2ps/aie2ps_encoder.h | Integrated custom sections into section writers during encoding |
| src/cpp/encoder/aie2/aie2_blob_encoder.h | Added custom section writers to AIE2 encoder output |
| src/cpp/elf/aie2ps/aie2ps_elfwriter.h | Included custom sections in ELF generation for AIE2PS |
| src/cpp/elf/aie2/aie2_blob_elfwriter.h | Included custom sections in ELF generation for AIE2 |
| src/cpp/common/writer.h | Added custom section storage and accessor methods to writer classes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const auto& pt_customer_sections = pt.get_child_optional("customer_section"); | ||
| if (pt_customer_sections) { | ||
| for (const auto& [sec_unused, section] : pt_customer_sections.get()) { |
There was a problem hiding this comment.
Corrected spelling of 'pt_customer_sections' variable name to 'pt_custom_sections' for consistency.
| const auto& pt_customer_sections = pt.get_child_optional("customer_section"); | |
| if (pt_customer_sections) { | |
| for (const auto& [sec_unused, section] : pt_customer_sections.get()) { | |
| const auto& pt_custom_sections = pt.get_child_optional("customer_section"); | |
| if (pt_custom_sections) { | |
| for (const auto& [sec_unused, section] : pt_custom_sections.get()) { |
| // Helper to parse customer_section array from JSON | ||
| std::map<std::string, std::vector<uint8_t>> parse_customer_sections( | ||
| const boost::property_tree::ptree& pt, | ||
| const std::vector<std::string>& paths) | ||
| { | ||
| std::map<std::string, std::vector<uint8_t>> custom_sections; | ||
| const auto& pt_customer_sections = pt.get_child_optional("customer_section"); | ||
| if (pt_customer_sections) { | ||
| for (const auto& [sec_unused, section] : pt_customer_sections.get()) { |
There was a problem hiding this comment.
Corrected spelling of 'customer_sections' to 'custom_sections'.
| // Helper to parse customer_section array from JSON | |
| std::map<std::string, std::vector<uint8_t>> parse_customer_sections( | |
| const boost::property_tree::ptree& pt, | |
| const std::vector<std::string>& paths) | |
| { | |
| std::map<std::string, std::vector<uint8_t>> custom_sections; | |
| const auto& pt_customer_sections = pt.get_child_optional("customer_section"); | |
| if (pt_customer_sections) { | |
| for (const auto& [sec_unused, section] : pt_customer_sections.get()) { | |
| // Helper to parse custom_section array from JSON | |
| std::map<std::string, std::vector<uint8_t>> parse_custom_sections( | |
| const boost::property_tree::ptree& pt, | |
| const std::vector<std::string>& paths) | |
| { | |
| std::map<std::string, std::vector<uint8_t>> custom_sections; | |
| const auto& pt_custom_sections = pt.get_child_optional("custom_section"); | |
| if (pt_custom_sections) { | |
| for (const auto& [sec_unused, section] : pt_custom_sections.get()) { |
| const auto& pt_customer_sections = pt.get_child_optional("customer_section"); | ||
| if (pt_customer_sections) { | ||
| for (const auto& [sec_unused, section] : pt_customer_sections.get()) { |
There was a problem hiding this comment.
Corrected spelling of 'pt_customer_sections' variable name to 'pt_custom_sections' for consistency.
| const auto& pt_customer_sections = pt.get_child_optional("customer_section"); | |
| if (pt_customer_sections) { | |
| for (const auto& [sec_unused, section] : pt_customer_sections.get()) { | |
| const auto& pt_custom_sections = pt.get_child_optional("customer_section"); | |
| if (pt_custom_sections) { | |
| for (const auto& [sec_unused, section] : pt_custom_sections.get()) { |
| const auto& pt_customer_sections = pic.get_child_optional("customer_section"); | ||
| if (pt_customer_sections) { | ||
| for (const auto& [sec_unused, section] : pt_customer_sections.get()) { |
There was a problem hiding this comment.
Corrected spelling of 'pt_customer_sections' variable name to 'pt_custom_sections' for consistency.
| const auto& pt_customer_sections = pic.get_child_optional("customer_section"); | |
| if (pt_customer_sections) { | |
| for (const auto& [sec_unused, section] : pt_customer_sections.get()) { | |
| const auto& pt_custom_sections = pic.get_child_optional("customer_section"); | |
| if (pt_custom_sections) { | |
| for (const auto& [sec_unused, section] : pt_custom_sections.get()) { |
| protected: // NOLINT | ||
| std::map<std::string, std::map<std::string, std::shared_ptr<asm_preprocessor_input>>> m_preprocessor_input; | ||
| // Global level custom sections | ||
| std::map<std::string, std::vector<uint8_t>> m_global_custom_sections; |
There was a problem hiding this comment.
warning: member variable 'm_global_custom_sections' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::map<std::string, std::vector<uint8_t>> m_global_custom_sections;
^| // Global level custom sections | ||
| std::map<std::string, std::vector<uint8_t>> m_global_custom_sections; | ||
| // Kernel level custom sections: kernel_name -> section_name -> data | ||
| std::map<std::string, std::map<std::string, std::vector<uint8_t>>> m_kernel_custom_sections; |
There was a problem hiding this comment.
warning: member variable 'm_kernel_custom_sections' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::map<std::string, std::map<std::string, std::vector<uint8_t>>> m_kernel_custom_sections;
^| // Kernel level custom sections: kernel_name -> section_name -> data | ||
| std::map<std::string, std::map<std::string, std::vector<uint8_t>>> m_kernel_custom_sections; | ||
| // Instance level custom sections: kernel_name -> instance_name -> section_name -> data | ||
| std::map<std::string, std::map<std::string, std::map<std::string, std::vector<uint8_t>>>> m_instance_custom_sections; |
There was a problem hiding this comment.
warning: member variable 'm_instance_custom_sections' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::map<std::string, std::map<std::string, std::map<std::string, std::vector<uint8_t>>>> m_instance_custom_sections;
^| protected: | ||
| std::map<std::string, std::vector<char>> m_data; | ||
| std::map<std::string, std::vector<char>> m_controlpkt; | ||
| std::map<std::string, std::vector<uint8_t>> m_custom_data; |
There was a problem hiding this comment.
warning: member variable 'm_custom_data' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::map<std::string, std::vector<uint8_t>> m_custom_data;
^| { | ||
| protected: // NOLINT | ||
| std::map<std::string, std::map<std::string, std::shared_ptr<asm_preprocessor_input>>> m_preprocessor_input; | ||
| // Global level custom sections |
There was a problem hiding this comment.
warning: member variable 'm_preprocessor_input' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::map<std::string, std::map<std::string, std::shared_ptr<asm_preprocessor_input>>> m_preprocessor_input;
^| std::map<std::string, std::map<std::string, std::shared_ptr<asm_preprocessor_input>>> m_preprocessor_input; | ||
| // Global level custom sections | ||
| std::map<std::string, std::vector<uint8_t>> m_global_custom_sections; | ||
| // Kernel level custom sections: kernel_name -> section_name -> data |
There was a problem hiding this comment.
warning: member variable 'm_global_custom_sections' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::map<std::string, std::vector<uint8_t>> m_global_custom_sections;
^| std::map<std::string, std::vector<uint8_t>> m_global_custom_sections; | ||
| // Kernel level custom sections: kernel_name -> section_name -> data | ||
| std::map<std::string, std::map<std::string, std::vector<uint8_t>>> m_kernel_custom_sections; | ||
| // Instance level custom sections: kernel_name -> instance_name -> section_name -> data |
There was a problem hiding this comment.
warning: member variable 'm_kernel_custom_sections' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::map<std::string, std::map<std::string, std::vector<uint8_t>>> m_kernel_custom_sections;
^| std::map<std::string, std::map<std::string, std::vector<uint8_t>>> m_kernel_custom_sections; | ||
| // Instance level custom sections: kernel_name -> instance_name -> section_name -> data | ||
| std::map<std::string, std::map<std::string, std::map<std::string, std::vector<uint8_t>>>> m_instance_custom_sections; | ||
|
|
There was a problem hiding this comment.
warning: member variable 'm_instance_custom_sections' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::map<std::string, std::map<std::string, std::map<std::string, std::vector<uint8_t>>>> m_instance_custom_sections;
^
sonals
left a comment
There was a problem hiding this comment.
This should not be so complicated. Can we not just add key/value pair API where the key is section name and value is the content? The client should deal with "Added parsing logic for "customer_section" arrays from JSON at global, kernel, and instance levels"
I am thinking the similar approach. We can just have the key(customer section name)/value(customer section blob) as a section in ELF. We can make use of the .group feature.
|
94bcd42 to
595576e
Compare
src/cpp/elf/elfwriter.cpp
Outdated
| sec_data.set_type(ELFIO::SHT_PROGBITS); | ||
| if (buffer->get_type() == code_section::custom) { | ||
| sec_data.set_type(SHT_CUSTOM_SECTION); | ||
| sec_data.set_info(info_index); |
There was a problem hiding this comment.
warning: narrowing conversion from 'ELFIO::Elf_Word' (aka 'unsigned int') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]
sec_data.set_info(info_index);
^| constexpr int phdr_align = 8; | ||
| constexpr int program_header_static_count = 2; | ||
| constexpr int program_header_dynamic_count = 3; | ||
| constexpr int SHT_CUSTOM_SECTION = ELFIO::SHT_LOUSER + 1; |
There was a problem hiding this comment.
warning: narrowing conversion from constant value 2147483649 (0x80000001) of type 'Elf_Word' (aka 'unsigned int') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]
constexpr int SHT_CUSTOM_SECTION = ELFIO::SHT_LOUSER + 1;
^|
@HimanshuChoudhary-Xilinx this looks much cleaner. Thanks!
|
Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
7a5f5ca to
48ef999
Compare
Update |
Looks good. One more question. Do we require customized sections in all level should have unique name? If so, we may want to put them into the ELF spec too. |
|
I still feel this is more specialized than it should be. Can we not have a simple key/value pair design where the key is defined by the user? The user can define the naming schema, etc. So essentially this reduces to supporting only the Global level. |
Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
sonals
left a comment
There was a problem hiding this comment.
There seems to be a lot of code duplication where we re-implement a logic for all the supported platform types. Can we not move the repeated methods to base classes?
Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
moved common code to common area |
| class config_writer_base : public writer | ||
| { | ||
| protected: | ||
| std::vector<std::shared_ptr<writer>> m_global_custom_sections; |
There was a problem hiding this comment.
warning: member variable 'm_global_custom_sections' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::vector<std::shared_ptr<writer>> m_global_custom_sections;
^| const file_artifact* m_artifacts = nullptr; | ||
| protected: // NOLINT | ||
| std::map<std::string, std::map<std::string, std::shared_ptr<asm_preprocessor_input>>> m_preprocessor_input; | ||
| global_custom_section_storage m_global_custom_sections; |
There was a problem hiding this comment.
warning: member variable 'm_global_custom_sections' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
global_custom_section_storage m_global_custom_sections;
^
Problem solved by the commit
Supporting custom section from config.json + testcase
sample json: test/aie2ps-ctrlcode/eff_net_coal/config.json in PR
Output: test/aie2ps-ctrlcode/eff_net_coal/eff_net_coal.gold in PR
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
How problem was solved, alternative solutions (if any) and why they were rejected
Risks (if any) associated the changes in the commit
What has been tested and how, request additional testing if necessary
Documentation impact (if any)
All custom section have section type "SHT_LOUSER+1"
customer_section->section_name is the section name in elf
Config.json
Global level