Skip to content

[DebugInfo] Correct an overly-restrictive REQUIRES clause. #116429

New issue

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

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

Already on GitHub? Sign in to your account

Conversation

CarlosAlbertoEnciso
Copy link
Member

@CarlosAlbertoEnciso CarlosAlbertoEnciso commented Nov 15, 2024

Include a regular expression in the 'REQUIRES' clause, to run
the test on all matching targets (x86_64 linux).

The original patch restricted to test just to 'x86_64-linux'
#116327

llvm#116327)

Consider the case when the compiler generates a static member. Any
consumer of the debug info generated for that case, would benefit if
that member has the DW_AT_artificial flag.

Fix the syntax for 'REQUIRES', to include a regular expression.
@llvmbot
Copy link
Member

llvmbot commented Nov 15, 2024

@llvm/pr-subscribers-debuginfo

Author: Carlos Alberto Enciso (CarlosAlbertoEnciso)

Changes

…. (#116327)

Consider the case when the compiler generates a static member. Any consumer of the debug info generated for that case, would benefit if that member has the DW_AT_artificial flag.

Fix the syntax for 'REQUIRES', to include a regular expression.


Full diff: https://github.com/llvm/llvm-project/pull/116429.diff

1 Files Affected:

  • (modified) llvm/test/DebugInfo/Generic/artificial-static-member.ll (+1-1)
diff --git a/llvm/test/DebugInfo/Generic/artificial-static-member.ll b/llvm/test/DebugInfo/Generic/artificial-static-member.ll
index 5c247d6959bf7b..47b7fa33b686f4 100644
--- a/llvm/test/DebugInfo/Generic/artificial-static-member.ll
+++ b/llvm/test/DebugInfo/Generic/artificial-static-member.ll
@@ -1,4 +1,4 @@
-; REQUIRES: x86_64-linux
+; REQUIRES: target={{x86_64-.*-linux}}
 ; RUN: llc -O0 -filetype=obj < %s |   \
 ; RUN: llvm-dwarfdump --debug-info - | FileCheck %s
 

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but you should write the commit message to be directly about what you're changing here, not carry with it the context of what the original patch did. Someone viewing this in isolation could be confused by that -- better to instead title it "Correct an overly-restrictive REQUIRES clause", another sentence of explanation, and then a link back to the original patch this landed in in case someone wants to read up on it.

@CarlosAlbertoEnciso CarlosAlbertoEnciso changed the title [DebugInfo] Add DW_AT_artificial for compiler generated static member… [DebugInfo] Correct an overly-restrictive REQUIRES clause. Nov 19, 2024
Include a regular expression in the 'REQUIRES' clause, to run
the test on all matching targets (x86_64 *linux*).

The original patch restricted to test just to 'x86_64-linux'
llvm#116327
@CarlosAlbertoEnciso
Copy link
Member Author

CarlosAlbertoEnciso commented Nov 19, 2024

@jmorse, @dwblaikie Thanks for your feedback.
Looking into more detail, the regular expression should be: {{x86_64-.*-linux.*}}
Updated title and commit comment to include link to original patch.

@CarlosAlbertoEnciso CarlosAlbertoEnciso merged commit 2e0a3c2 into llvm:main Nov 19, 2024
8 checks passed
@CarlosAlbertoEnciso CarlosAlbertoEnciso deleted the artificial-static-member-fix-triple branch November 25, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants