google / xls

XLS: Accelerated HW Synthesis
http://google.github.io/xls/
Apache License 2.0
1.22k stars 181 forks source link

[XLSCC] Hard crash when using parameter pack in class constructor #620

Closed mtdudek closed 2 years ago

mtdudek commented 2 years ago

Hi,

I found issue with translation class constructors with parameter pack syntax. Example of such constructor:

template<typename...Ranges>
    AXI4LiteWorker(Ranges... b): ranges{b...}

Issue was due to XLScc creating n parameters with name b, causing name collision and then crash. Tracking down this issue was not that easy, error message from FunctionBuilder::Param got lost somewhere on the way. Please find my patch proposition in attachment: suggestion.txt

spurserh commented 2 years ago

Reproduced with simple example, confirmed that the duplicate parameter name is the issue:

int inner(int a, int b) { return a+2*b; }

template int Test(Ranges... b) { return inner(b...); }

pragma hls_top

int sum(int a, int b) { return Test<int,int>(a,b); }

Stack trace:

6 0x000055556278891c in xls::logging_internal::LogMessageFatal::~LogMessageFatal (this=0x7fffffff0440) at third_party/xls/common/logging/log_message.cc:475

7 0x000055555a859375 in xlscc::CValue::CValue (this=0x7fffffff1d38, rvalue=..., type=std::shared_ptr is nullptr, disable_type_check=true, lvalue=std::shared_ptr is nullptr)

at ./third_party/xls/contrib/xlscc/translator.h:364

8 0x000055555a80e13c in xlscc::Translator::GenerateIR_Function (this=0x7fffffffd808, funcdecl=0x14e53f9ec040, name_override=std::string_view of length 0: "", force_static=false)

at third_party/xls/contrib/xlscc/translator.cc:1210

9 0x000055555a82541e in xlscc::Translator::TranslateFunctionToXLS (this=0x7fffffffd808, decl=0x14e53f9ec040) at third_party/xls/contrib/xlscc/translator.cc:2433

10 0x000055555a827a1f in xlscc::Translator::GenerateIR_Call (this=0x7fffffffd808, funcdecl=0x14e53f9ec040, expr_args=std::vector of length 2, capacity 2 = {...}, this_inout=0x0, loc=...)

at third_party/xls/contrib/xlscc/translator.cc:2645

11 0x000055555a82605d in xlscc::Translator::GenerateIR_Call (this=0x7fffffffd808, call=0x14e53f9ec230, loc=...) at third_party/xls/contrib/xlscc/translator.cc:2522

12 0x000055555a8067f8 in xlscc::Translator::GenerateIR_Expr (this=0x7fffffffd808, expr=0x14e53f9ec230, loc=...) at third_party/xls/contrib/xlscc/translator.cc:3042

13 0x000055555a831f4d in xlscc::Translator::GenerateIR_Stmt (this=0x7fffffffd808, stmt=0x14e53f9ec290, ctx=...) at third_party/xls/contrib/xlscc/translator.cc:3743

14 0x000055555a8117c0 in xlscc::Translator::GenerateIR_Compound (this=0x7fffffffd808, body=0x14e53f9ec2a0, ctx=...) at third_party/xls/contrib/xlscc/translator.cc:3710

15 0x000055555a80f8f9 in xlscc::Translator::GenerateIR_Function (this=0x7fffffffd808, funcdecl=0x14e53f99fc78, name_override=std::string_view of length 3: "sum", force_static=false)

at third_party/xls/contrib/xlscc/translator.cc:1279

16 0x000055555a8468a5 in xlscc::Translator::GenerateIR_Top_Function (this=0x7fffffffd808, package=0x7fffffffce78, force_static=false) at third_party/xls/contrib/xlscc/translator.cc:5068

17 0x000055555a7dfeea in xlscc::Run (cpp_path=std::string_view of length 12: "/tmp/main.cc") at third_party/xls/contrib/xlscc/main.cc:164

18 0x000055555a7e1597 in main (argc=2, argv=0x7fffffffdc48) at third_party/xls/contrib/xlscc/main.cc:239

mtdudek commented 2 years ago

Thank you for looking into this issue. I've tested my suggested patch locally, and it did work, but I think it's more like a hack than proper fix

spurserh commented 2 years ago

I have created a fix internally. It is pending code review. You should see it come through tomorrow-ish.

Thanks a lot for the bug report!

spurserh commented 2 years ago

Review was quick. I guess external users are motivational. https://github.com/google/xls/commit/515ed2e58601a5b4950a0f5d93b0ed3db484b2d1