Add __attribute__((warn_unused_result)) to LLVMErrorRef

[dblaikie]

Summary

Sending this out a bit half-baked unfortunately - we added support for
this context a couple of years ago in D102122 (which turned up in clang 15
onwards: https://godbolt.org/z/haz8qrTT3 ), not sure if the macro detection
will correctly detect that the attribute is usable in a typedef context...

[MaskRay]

Useful in my view:)

[aaron.ballman]

llvm/examples/OrcV2Examples/OrcV2CBindingsLazy/OrcV2CBindingsLazy.c

198

Keep in mind, I know nothing about Orc. :-D

It seems that the only in-tree uses of this API that check results are the unit tests. Perhaps this function's return type is inappropriate? Or does this suggest we should be marking individual APIs instead of the return type itself?

llvm/include/llvm-c/ExternC.h

47–51

Hmm, worth it to add __has_c_attribute(nodiscard) as a branch, or given C2x's newness, not worth it?

[dblaikie]

llvm/include/llvm-c/ExternC.h

47–51

Does C2x support the attribute on typedefs? (the C++ committee wasn't so enthusiastic about that change & I don't follow the C committee directly)

I'm happy enough to include C support, if it applies.

[aaron.ballman]

llvm/include/llvm-c/ExternC.h

47–51

Ah good point, it's not allowed on typedefs there either. Neeeevermind. :-)

[dblaikie]

llvm/include/llvm-c/ExternC.h

47–51

could rename the macro WARN_UNUSED_RESULT rather than NODISCARD_GNU_STYLE?

[aaron.ballman]

llvm/include/llvm-c/ExternC.h

47–51

Hmmm, either seems about equally as confusing. What about LLVM_NODISCARD_TYPEDEF to make it clear that this should be used for typedefs and leave it to another macro to handle non-discardable function markings?

[dblaikie]

llvm/include/llvm-c/ExternC.h

47–51

I guess, sure.

I guess this also needs a more narrow macro check to verify the clang version (since it seems we support back to clang 5, and this only works on clang 15 and above) - I can't seem to find an example of a macro check for clang version in any of our Compiler.h and similar macro-y/compat headers in LLVM - do you happen to know of an example off-hand?

[lhames]

Adding warn-unused attribute(s) either to LLVMErrorRef (if possible) or to the individual APIs seems like a great idea to me. No opinion on the mechanics -- I don't know anything about the relevant C attributes.

llvm/examples/OrcV2Examples/OrcV2CBindingsLazy/OrcV2CBindingsLazy.c

198

The return type is appropriate, I've just been lazy about checking it -- adding warn-unused seems like a big win for that reason. :)

Rather than casting to void I should add an LLVMCantFail macro that asserts that the result is LLVMErrorSuccess.

llvm/examples/OrcV2Examples/OrcV2CBindingsLazy/OrcV2CBindingsLazy.c

LLVMOrcCSymbolAliasMapPair ReExports[2] = {
       {LLVMOrcLLJITMangleAndIntern(J, "bar"),
        {LLVMOrcLLJITMangleAndIntern(J, "bar_body"), flag}},
   };
 
   {
     LLVMOrcJITDylibRef MainJD = LLVMOrcLLJITGetMainJITDylib(J);
     LLVMOrcMaterializationUnitRef MU =
         LLVMOrcLazyReexports(LCTM, ISM, MainJD, ReExports, 2);
-    LLVMOrcJITDylibDefine(MainJD, MU);
+    (void)LLVMOrcJITDylibDefine(MainJD, MU);
   }
 
   // Look up the address of our demo entry point.
   LLVMOrcJITTargetAddress EntryAddr;
   {
     LLVMErrorRef Err;
     if ((Err = LLVMOrcLLJITLookup(J, &EntryAddr, "entry"))) {
       MainResult = handleError(Err);

llvm/examples/OrcV2Examples/OrcV2CBindingsVeryLazy/OrcV2CBindingsVeryLazy.c

LLVMOrcMaterializationUnitRef FooMU =
         LLVMOrcCreateCustomMaterializationUnit("FooMU", J, &FooSym, 1, NULL,
                                                &Materialize, NULL, &Destroy);
 
     LLVMOrcMaterializationUnitRef BarMU =
         LLVMOrcCreateCustomMaterializationUnit("BarMU", J, &BarSym, 1, NULL,
                                                &Materialize, NULL, &Destroy);
 
     LLVMOrcJITDylibRef MainJD = LLVMOrcLLJITGetMainJITDylib(J);
-    LLVMOrcJITDylibDefine(MainJD, FooMU);
-    LLVMOrcJITDylibDefine(MainJD, BarMU);
+    (void)LLVMOrcJITDylibDefine(MainJD, FooMU);
+    (void)LLVMOrcJITDylibDefine(MainJD, BarMU);
   }
 
   // add lazy reexports
   LLVMOrcIndirectStubsManagerRef ISM =
       LLVMOrcCreateLocalIndirectStubsManager(TargetTriple);
 
   LLVMOrcLazyCallThroughManagerRef LCTM;
   {
LLVMOrcCSymbolAliasMapPair ReExports[2] = {
       {LLVMOrcLLJITMangleAndIntern(J, "bar"),
        {LLVMOrcLLJITMangleAndIntern(J, "bar_body"), Flags}},
   };
 
   {
     LLVMOrcJITDylibRef MainJD = LLVMOrcLLJITGetMainJITDylib(J);
     LLVMOrcMaterializationUnitRef MU =
         LLVMOrcLazyReexports(LCTM, ISM, MainJD, ReExports, 2);
-    LLVMOrcJITDylibDefine(MainJD, MU);
+    (void)LLVMOrcJITDylibDefine(MainJD, MU);
   }
 
   // Look up the address of our demo entry point.
   LLVMOrcJITTargetAddress EntryAddr;
   {
     LLVMErrorRef Err;
     if ((Err = LLVMOrcLLJITLookup(J, &EntryAddr, "entry"))) {
       MainResult = handleError(Err);

llvm/include/llvm-c/Error.h

  * @{
  */
 
 #define LLVMErrorSuccess 0
 
 /**
  * Opaque reference to an error instance. Null serves as the 'success' value.
  */
-typedef struct LLVMOpaqueError *LLVMErrorRef;
+LLVM_NODISCARD_GNU_STYLE_CLANG typedef struct LLVMOpaqueError *LLVMErrorRef;
 
 /**
  * Error type identifier.
  */
 typedef const void *LLVMErrorTypeId;
 
 /**
  * Returns the type id for the given error instance, which must be a failure

llvm/include/llvm-c/ExternC.h

 #define LLVM_C_EXTERN_C_END                                                    \
   LLVM_C_STRICT_PROTOTYPES_END                                                 \
   }
 #else
 #define LLVM_C_EXTERN_C_BEGIN LLVM_C_STRICT_PROTOTYPES_BEGIN
 #define LLVM_C_EXTERN_C_END LLVM_C_STRICT_PROTOTYPES_END
 #endif
 
+#ifndef LLVM_HAS_ATTRIBUTE
+#if defined(__has_attribute)
+#define LLVM_HAS_ATTRIBUTE(x) __has_attribute(x)
+#else
+#define LLVM_HAS_ATTRIBUTE(x) 0
+#endif
+#endif
+
+#if LLVM_HAS_ATTRIBUTE(warn_unused_result) && defined(__clang__)
+#define LLVM_NODISCARD_GNU_STYLE_CLANG __attribute__((warn_unused_result))
+#else
+#define LLVM_NODISCARD_GNU_STYLE_CLANG
+#endif
+
 #endif

llvm/unittests/ExecutionEngine/Orc/OrcCAPITest.cpp

static void SetUpTestCase() {
     LLVMOrcLLJITRef J;
     if (LLVMErrorRef E = LLVMOrcCreateLLJIT(&J, Builder)) {
       // If setup fails then disable these tests.
       TargetSupported = false;
       LLVMConsumeError(E);
       return;
     }
 
-    LLVMOrcDisposeLLJIT(J);
+    (void)LLVMOrcDisposeLLJIT(J);
     TargetSupported = true;
   }
 
   void SetUp() override {
     if (!TargetSupported)
       GTEST_SKIP();
 
     LLVMOrcJITTargetMachineBuilderRef JTMB = nullptr;
void SetUp() override {
     (void)E2;
     ExecutionSession = LLVMOrcLLJITGetExecutionSession(Jit);
     MainDylib = LLVMOrcLLJITGetMainJITDylib(Jit);
   }
   void TearDown() override {
     // Check whether Jit has already been torn down -- we allow clients to do
     // this manually to check teardown behavior.
     if (Jit) {
-      LLVMOrcDisposeLLJIT(Jit);
+      (void)LLVMOrcDisposeLLJIT(Jit);
       Jit = nullptr;
     }
   }
 
 protected:
   static bool isSupported(StringRef Triple) {
     // TODO: Print error messages in failure logs, use them to audit this list.
     // Some architectures may be unsupportable or missing key components, but
#endif
   if (LLVMErrorRef E = LLVMOrcLLJITAddLLVMIRModuleWithRT(Jit, RT, TSM))
     FAIL() << "Failed to add LLVM IR module to LLJIT (triple = " << TargetTriple
            << "): " << toString(E);
   LLVMOrcJITTargetAddress TestFnAddr;
   if (LLVMErrorRef E = LLVMOrcLLJITLookup(Jit, &TestFnAddr, "sum"))
     FAIL() << "Symbol \"sum\" was not added into JIT (triple = " << TargetTriple
            << "): " << toString(E);
   ASSERT_TRUE(!!TestFnAddr);
-  LLVMOrcResourceTrackerRemove(RT);
+  (void)LLVMOrcResourceTrackerRemove(RT);
   LLVMOrcJITTargetAddress OutAddr;
   LLVMErrorRef Err = LLVMOrcLLJITLookup(Jit, &OutAddr, "sum");
   ASSERT_TRUE(Err);
   LLVMConsumeError(Err);
 
   ASSERT_FALSE(OutAddr);
   LLVMOrcReleaseResourceTracker(RT);
 }
LLVMOrcSymbolStringPoolEntryRef DependencySymbol =
       LLVMOrcLLJITMangleAndIntern(J, "dependency");
 
   LLVMOrcRetainSymbolStringPoolEntry(OtherSymbol);
   LLVMOrcRetainSymbolStringPoolEntry(DependencySymbol);
   LLVMOrcCSymbolFlagsMapPair NewSymbols[] = {
       {OtherSymbol, Flags},
       {DependencySymbol, Flags},
   };
-  LLVMOrcMaterializationResponsibilityDefineMaterializing(MR, NewSymbols, 2);
+  (void)LLVMOrcMaterializationResponsibilityDefineMaterializing(MR, NewSymbols, 2);
 
   LLVMOrcRetainSymbolStringPoolEntry(OtherSymbol);
   LLVMOrcMaterializationResponsibilityRef OtherMR = NULL;
   {
     LLVMErrorRef Err = LLVMOrcMaterializationResponsibilityDelegate(
         MR, &OtherSymbol, 1, &OtherMR);
     if (Err) {
       char *ErrMsg = LLVMGetErrorMessage(Err);
void Materialize(void *Ctx, LLVMOrcMaterializationResponsibilityRef MR) {
   LLVMOrcMaterializationResponsibilityAddDependencies(MR, TargetSym.Name,
                                                       &Dependency, 1);
 
   LLVMOrcRetainSymbolStringPoolEntry(DependencySymbol);
   LLVMOrcMaterializationResponsibilityAddDependenciesForAll(MR, &Dependency, 1);
 
   // See FIXME above
   LLVMOrcCSymbolMapPair Pair = {DependencySymbol, Sym};
-  LLVMOrcMaterializationResponsibilityNotifyResolved(MR, &Pair, 1);
+  (void)LLVMOrcMaterializationResponsibilityNotifyResolved(MR, &Pair, 1);
   // DependencySymbol no longer owned by us
 
   Pair = {TargetSym.Name, Sym};
-  LLVMOrcMaterializationResponsibilityNotifyResolved(MR, &Pair, 1);
+  (void)LLVMOrcMaterializationResponsibilityNotifyResolved(MR, &Pair, 1);
 
-  LLVMOrcMaterializationResponsibilityNotifyEmitted(MR);
+  (void)LLVMOrcMaterializationResponsibilityNotifyEmitted(MR);
   LLVMOrcDisposeMaterializationResponsibility(MR);
 }
 
 TEST_F(OrcCAPITestBase, MaterializationResponsibility) {
   LLVMJITSymbolFlags Flags = {
       LLVMJITSymbolGenericFlagsExported | LLVMJITSymbolGenericFlagsCallable, 0};
   LLVMOrcCSymbolFlagsMapPair Sym = {LLVMOrcLLJITMangleAndIntern(Jit, "foo"),
                                     Flags};
 
   LLVMOrcMaterializationUnitRef MU = LLVMOrcCreateCustomMaterializationUnit(
       "MU", (void *)Jit, &Sym, 1, NULL, &Materialize, NULL, &Destroy);
   LLVMOrcJITDylibRef JD = LLVMOrcLLJITGetMainJITDylib(Jit);
-  LLVMOrcJITDylibDefine(JD, MU);
+  (void)LLVMOrcJITDylibDefine(JD, MU);
 
   LLVMOrcJITTargetAddress Addr;
   if (LLVMErrorRef Err = LLVMOrcLLJITLookup(Jit, &Addr, "foo")) {
     FAIL() << "foo was not materialized " << toString(Err);
   }
   ASSERT_TRUE(!!Addr);
   ASSERT_EQ(Addr, (LLVMOrcJITTargetAddress)&TargetFn);
 
TEST_F(OrcCAPITestBase, SuspendedLookup) {
 
   // Check that the query completed.
   EXPECT_TRUE(Ctx.QueryCompleted);
 
   // Release our local copy of the string.
   LLVMOrcReleaseSymbolStringPoolEntry(Ctx.NameToGenerate);
 
   // Explicitly tear down the JIT.
-  LLVMOrcDisposeLLJIT(Jit);
+  (void)LLVMOrcDisposeLLJIT(Jit);
   Jit = nullptr;
 
   // Check that the generator context was "destroyed".
   EXPECT_TRUE(Ctx.Disposed);
 }