Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Open
jcortell68 opened this issue Sep 26, 2024 · 1 comment

Comments

@jcortell68
Copy link
Contributor

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
Copy link
Contributor

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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants