Committed: https://gcc.gnu.org/cgit/gcc/commit/?id=622b68c6e14
Thanks Tamar! > On 27 Feb 2026, at 1:45 PM, Tamar Christina <[email protected]> wrote: > > External email: Use caution opening links or attachments > > >> -----Original Message----- >> From: Soumya AR <[email protected]> >> Sent: 26 February 2026 09:39 >> To: [email protected] >> Cc: Tamar Christina <[email protected]>; Kyrylo Tkachov >> <[email protected]>; David Malcolm <[email protected]> >> Subject: [PATCH] aarch64: Fix NULL structures in base tunings being silently >> ignored by JSON parser >> >> In the JSON parser's parse_object_helper for const pointer members, we have >> a >> if (!member) return; guard at entry. >> >> This is because the function works by dereferencing the member pointer and >> making a non-const copy of the object it points to. The parser can then >> override this copy with new values and re-assign the pointer to the new >> object. >> >> This is problematic if member is NULL in the base tunings, but provided in >> the >> JSON file, which can happen with some combinations of -mcpu and >> -muser-provided-CPU. >> >> For example, with no -mcpu, if the generic tunings has issue_info set to >> nullptr (eg. generic_armv8_a), then even if the user does provide issue_info >> data in the JSON file, parse_object_helper will silently ignore it. >> >> The naive fix for this is to create a zero-initialized copy of the object if >> it is NULL, and then override it with the new values from the JSON file. >> >> However, this results in another problem: if the user provides only selective >> fields of the strucutre, the rest of the fields will be set to zero and >> potentially interfere with costing decisions. >> >> I think at that point the best we can do is emit a warning. With David >> Malcolm's >> improved JSON diagnostics, we can be specific about the problematic >> structure as >> well. > > Agreed. > >> >> ---- >> >> Since we potentially zero-initialize objects, I had to add a default >> constructor >> to objects that only had parameterized constructors. Is this OK? > > I think it's OK since the normal usage of them are through the header files > which > fully populates them or explicitly sets to NULL. So don't think any usages > would > break. > >> >> Bootstrapped and regtested on aarch64-linux-gnu, OK for trunk? >> > > OK. > > Thanks, > Tamar > >> Signed-off-by: Soumya AR <[email protected]> >> >> gcc/ChangeLog: >> >> * config/aarch64/aarch64-json-tunings-parser.cc >> (parse_object_helper): >> Zero-initialize objects that are NULL in the base tunings, if provided >> in JSON tunings. >> * config/aarch64/aarch64-protos.h (struct sve_vec_cost): Add default >> constructor. >> (struct aarch64_simd_vec_issue_info): Likewise. >> (struct aarch64_sve_vec_issue_info): Likewise. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/aarch64/aarch64-json-tunings/nullptr-issue-info.c: >> New test. >> * gcc.target/aarch64/aarch64-json-tunings/nullptr-issue-info.json: >> New test input. >> --- >> .../aarch64/aarch64-json-tunings-parser.cc | 8 ++-- >> gcc/config/aarch64/aarch64-protos.h | 6 +++ >> .../aarch64-json-tunings/nullptr-issue-info.c | 20 ++++++++++ >> .../nullptr-issue-info.json | 38 +++++++++++++++++++ >> 4 files changed, 68 insertions(+), 4 deletions(-) >> create mode 100644 gcc/testsuite/gcc.target/aarch64/aarch64-json- >> tunings/nullptr-issue-info.c >> create mode 100644 gcc/testsuite/gcc.target/aarch64/aarch64-json- >> tunings/nullptr-issue-info.json >> >> diff --git a/gcc/config/aarch64/aarch64-json-tunings-parser.cc >> b/gcc/config/aarch64/aarch64-json-tunings-parser.cc >> index 9b7bdf535c9..19598034761 100644 >> --- a/gcc/config/aarch64/aarch64-json-tunings-parser.cc >> +++ b/gcc/config/aarch64/aarch64-json-tunings-parser.cc >> @@ -109,9 +109,6 @@ static std::enable_if_t<std::is_pointer<T>::value >> parse_object_helper (const json::object *field_obj, T &member, >> parse_func_type<T> parse_func) >> { >> - if (!member) >> - return; >> - >> /* Use static storage for the non-const copy. >> This works because tune_params does not have nested structures of the >> same type, but has room for errors if we end up having pointers to the >> @@ -125,7 +122,10 @@ parse_object_helper (const json::object *field_obj, >> T &member, >> } >> already_initialized = true; >> using NonConstType = std::remove_const_t<std::remove_pointer_t<T>>; >> - static NonConstType new_obj = *member; >> + if (!member) >> + warning (0, "JSON tuning overrides an unspecified structure in the base >> " >> + "tuning; fields not provided in JSON will default to 0"); >> + static NonConstType new_obj = member ? *member : NonConstType{}; >> parse_func (field_obj, new_obj); >> member = &new_obj; >> } >> diff --git a/gcc/config/aarch64/aarch64-protos.h >> b/gcc/config/aarch64/aarch64-protos.h >> index d1f2873f208..3f359b0069d 100644 >> --- a/gcc/config/aarch64/aarch64-protos.h >> +++ b/gcc/config/aarch64/aarch64-protos.h >> @@ -255,6 +255,8 @@ typedef struct simd_vec_cost advsimd_vec_cost; >> /* SVE-specific extensions to the information provided by simd_vec_cost. */ >> struct sve_vec_cost : simd_vec_cost >> { >> + sve_vec_cost () = default; >> + >> CONSTEXPR sve_vec_cost (const simd_vec_cost &base, >> unsigned int clast_cost, >> unsigned int fadda_f16_cost, >> @@ -365,6 +367,8 @@ using aarch64_scalar_vec_issue_info = >> aarch64_base_vec_issue_info; >> Advanced SIMD and SVE. */ >> struct aarch64_simd_vec_issue_info : aarch64_base_vec_issue_info >> { >> + aarch64_simd_vec_issue_info () = default; >> + >> CONSTEXPR aarch64_simd_vec_issue_info (aarch64_base_vec_issue_info >> base, >> unsigned int ld2_st2_general_ops, >> unsigned int ld3_st3_general_ops, >> @@ -393,6 +397,8 @@ using aarch64_advsimd_vec_issue_info = >> aarch64_simd_vec_issue_info; >> is a concept of "predicate operations". */ >> struct aarch64_sve_vec_issue_info : aarch64_simd_vec_issue_info >> { >> + aarch64_sve_vec_issue_info () = default; >> + >> CONSTEXPR aarch64_sve_vec_issue_info >> (aarch64_simd_vec_issue_info base, >> unsigned int pred_ops_per_cycle, >> diff --git a/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr- >> issue-info.c b/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr- >> issue-info.c >> new file mode 100644 >> index 00000000000..3984c63b39e >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr-issue- >> info.c >> @@ -0,0 +1,20 @@ >> +/* Test that a structure provided in JSON is parsed even when the base >> tuning >> + model has the structure set to NULL. */ >> + >> +/* { dg-do compile } */ >> +/* { dg-additional-options "-muser-provided- >> CPU=${srcdir}/gcc.target/aarch64/aarch64-json-tunings/nullptr-issue- >> info.json -fdump-tuning-model=temp-nullptr-issue-info.json" } */ >> + >> +/* { dg-warning "JSON tuning overrides an unspecified structure in the base >> tuning; fields not provided in JSON will default to 0" "" { target *-*-* } 0 >> } */ >> +/* { dg-warning "JSON tuning file does not contain version information" "" { >> target *-*-* } 0 } */ >> + >> +/* { dg-final { scan-file "temp-nullptr-issue-info.json" >> "\"loads_stores_per_cycle\": 4" } } */ >> +/* { dg-final { scan-file "temp-nullptr-issue-info.json" >> "\"general_ops_per_cycle\": 8" } } */ >> + >> +/* { dg-final { scan-file "temp-nullptr-issue-info.json" >> "\"ld2_st2_general_ops\": 2" } } */ >> +/* { dg-final { scan-file "temp-nullptr-issue-info.json" >> "\"ld4_st4_general_ops\": 3" } } */ >> + >> +/* { dg-final { scan-file "temp-nullptr-issue-info.json" >> "\"pred_ops_per_cycle\": 2" } } */ >> +/* { dg-final { scan-file "temp-nullptr-issue-info.json" >> "\"while_pred_ops\": >> 1" } } */ >> +/* { dg-final { scan-file "temp-nullptr-issue-info.json" >> "\"gather_scatter_pair_general_ops\": 1" } } */ >> + >> +int main () {} >> diff --git a/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr- >> issue-info.json b/gcc/testsuite/gcc.target/aarch64/aarch64-json- >> tunings/nullptr-issue-info.json >> new file mode 100644 >> index 00000000000..b6fefa87544 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/aarch64/aarch64-json-tunings/nullptr-issue- >> info.json >> @@ -0,0 +1,38 @@ >> +{ >> + "tune_params": { >> + "vec_costs": { >> + "issue_info": { >> + "scalar": { >> + "loads_stores_per_cycle": 4, >> + "stores_per_cycle": 2, >> + "general_ops_per_cycle": 8, >> + "fp_simd_load_general_ops": 0, >> + "fp_simd_store_general_ops": 1 >> + }, >> + "advsimd": { >> + "loads_stores_per_cycle": 3, >> + "stores_per_cycle": 2, >> + "general_ops_per_cycle": 6, >> + "fp_simd_load_general_ops": 0, >> + "fp_simd_store_general_ops": 1, >> + "ld2_st2_general_ops": 2, >> + "ld3_st3_general_ops": 2, >> + "ld4_st4_general_ops": 3 >> + }, >> + "sve": { >> + "loads_stores_per_cycle": 3, >> + "stores_per_cycle": 2, >> + "general_ops_per_cycle": 6, >> + "fp_simd_load_general_ops": 0, >> + "fp_simd_store_general_ops": 1, >> + "pred_ops_per_cycle": 2, >> + "while_pred_ops": 1, >> + "int_cmp_pred_ops": 0, >> + "fp_cmp_pred_ops": 0, >> + "gather_scatter_pair_general_ops": 1, >> + "gather_scatter_pair_pred_ops": 1 >> + } >> + } >> + } >> + } >> +} >> -- >> 2.43.0
