GPUOpen-Tools / gpu_performance_api

GPU Performance API for AMD GPUs
MIT License
251 stars 46 forks source link

Public Counter Compiler fails awkwardly if public counter definition file has an uppercase character in it #84

Open jcortell68 opened 2 months ago

jcortell68 commented 2 months ago

The failure is easy to reproduce. Add an empty file

source\auto_generated\public_counter_compiler_input_files\public_counter_names_vk_gfxWhatever.txt

then run the Public Counter Compiler with API=VK and GPU Family=gfxWhatever. You'll end up with this error:

Compiling API VK for GPU Family gfxWhatever
Info: Unable to find file C:\j.cortell\git\gpu_performance_api\source\public_counter_compiler_input_files\public_counter_definitions_gfxwhatever_ter_names_vk_gfxwhatever.txt

That message is a bit long. Notice the simple filename that it says doesn't exist:

public_counter_definitions_gfxwhatever_ter_names_vk_gfxwhatever.txt

You can see it's malformed. The reason is that the logic in PublicCounterCompiler::CounterCompiler::CompileCounters() assumes that the files in source\public_counter_compiler_input_files have only lowercase characters in their names. That unchecked assumption, if false, results in a -1 value for asicIndex here. The code then proceeds to use the -1, resulting in the malformed filename in the error message.

At a minimum, it would be nice if the compiler failed with a comprehensible and actionable message if someone introduces a file with an uppercase character. The following addition produces a nice error message for me:

                 foreach (string counterNamesFile in baseGfxFileNames)
                 {
+                    string filename = Path.GetFileName(counterNamesFile);
+                    if (filename != filename.ToLower())
+                    {
+                        errorHandler("Public counter name file has uppercase characters in it: " + counterNamesFile);
+                        return false;
+                    }
+
                     fileNames.Add(counterNamesFile);
                 }

Of course, the alternative is to make the code work even if the file has an uppercase character in it. I'm not sure, though, if that would require adjustments in a bunch of places. I think it's perfectly fine to require the files to be all lowercase; the compiler just needs to tell the user when that requirement isn't met. I.e., the above would be a sufficient and safe fix in my book. The nonsensical message it is producing today isn't helpful and took some time to debug to discover what was really wrong.

PLohrmannAMD commented 1 month ago

Thank you for bringing this to our attention, and the thorough analysis! We follow the Google C++ Style Guide (https://google.github.io/styleguide/cppguide.html) which recommends lower case file names, but I think our code generation tools should be robust enough to handle upper case names as well. Certainly, reporting an error as you've suggested is a minimal requirement.