[libunwind] Compile with -Wunused-but-set-variable

[danielkiss]

Summary

-Wunused-but-set-variable triggers a warning even the block of code is effectively dead.

[MaskRay]

Thanks!

libunwind/CMakeLists.txt

198

Place below other -Wunused-*

[danielkiss]

libunwind/CMakeLists.txt

198

added in ABC order.

[dblaikie]

any chance this can inherit flag settings from llvm, so we can make such configuration choices in one place?

[danielkiss]

In D107835#2948054, @dblaikie wrote:

any chance this can inherit flag settings from llvm, so we can make such configuration choices in one place?

-Wunused-but-set-variable just added to unwind to be sure warnings won't come back. I don't see where it might cause problems. IIRC global runtime flags will be included in uninwd's cflags.

[dblaikie]

In D107835#2974586, @danielkiss wrote:

In D107835#2948054, @dblaikie wrote:

any chance this can inherit flag settings from llvm, so we can make such configuration choices in one place?

-Wunused-but-set-variable just added to unwind to be sure warnings won't come back. I don't see where it might cause problems. IIRC global runtime flags will be included in uninwd's cflags.

Sorry, I'm having trouble understanding what you mean - could you rephrase/use a few more words? (if the flag isn't added to libunwind, the warnings wouldn't come back because the warning wouldn't be enabled? The problems I have is that maintaining different flag sets in different subprojects (even if they're only additive) is more work/creates an inconsistent experience across subprojects compared to having all the flags selected in llvm proper so they're consistently used by all subprojects)

[MaskRay]

Some users do standalone builds. They use libunwind/CMakeLists.txt as the main config, skipping llvm/CMakeLists.txt.

cmake -GNinja -Hlibunwind -B/tmp/out/play In this setting, some llvm/CMakeLists.txt options (e.g. -fvisibility-inlines-hidden) are not inherited to libunwind.

Note that libcxx sets its own options as well as inherits options from llvm/CMakeLists.txt. I am on the fence whether libunwind should inherit llvm/'s options.

[dblaikie]

In D107835#2974899, @MaskRay wrote:

Some users do standalone builds. They use libunwind/CMakeLists.txt as the main config, skipping llvm/CMakeLists.txt.

cmake -GNinja -Hlibunwind -B/tmp/out/play In this setting, some llvm/CMakeLists.txt options (e.g. -fvisibility-inlines-hidden) are not inherited to libunwind.

Note that libcxx sets its own options as well as inherits options from llvm/CMakeLists.txt. I am on the fence whether libunwind should inherit llvm/'s options.

Not an absolute requirement - but I did want to check if it was possible/reasonable. If this is only to support parity in a standalone build, perhaps we should document/comment that the list should be kept in sync with some list in the LLVM build files?

[MaskRay]

Looks like the libunwind warning list is stronger than llvm's (scattered in llvm/cmake/modules/HandleLLVMOptions.cmake).
I think it is probably fine to not duplicate options which are also set in llvm/.

Perhaps not many groups use standalone builds, so losing some warning options is fine.

llvm/'s warnings are under LLVM_ENABLE_WARNINGS, while libunwind specifics are unconditional. Now I am unsure what to do.

[dblaikie]

In D107835#2975019, @MaskRay wrote:

Looks like the libunwind warning list is stronger than llvm's (scattered in llvm/cmake/modules/HandleLLVMOptions.cmake).
I think it is probably fine to not duplicate options which are also set in llvm/.

I don't mind setting them if someone wants standalone builds to have more parity with non-standalone. But I'd object more to adding extra warnings per project without a pretty significant reason that one subproject should differ from others.

Perhaps not many groups use standalone builds, so losing some warning options is fine.

llvm/'s warnings are under LLVM_ENABLE_WARNINGS, while libunwind specifics are unconditional. Now I am unsure what to do.

If this flag isn't turned on for non-standalone builds/turned on in LLVM, then I'm not sure what issue the patch is intending to address. There wouldn't be any inconsistency, there wouldn't be any regression issue (like some builds checking the warning and some not checking it - so someone in the non-checking build committing a warning violation, breaking the checking build) if the flag wasn't added?

But if someone wants to turn it on for the non-standalone build, that sounds OK. It's probably not especially painful to cleanup.

[MaskRay]

In D107835#2975100, @dblaikie wrote:

In D107835#2975019, @MaskRay wrote:

Looks like the libunwind warning list is stronger than llvm's (scattered in llvm/cmake/modules/HandleLLVMOptions.cmake).
I think it is probably fine to not duplicate options which are also set in llvm/.

I don't mind setting them if someone wants standalone builds to have more parity with non-standalone. But I'd object more to adding extra warnings per project without a pretty significant reason that one subproject should differ from others.

Agree.

For -Wunused-but-set-variable in libunwind, there is probably another justification: libunwind uses -Wunused-parameter while llvm/cmake/modules/HandleLLVMOptions.cmake disables it.
Since libunwind is -Wunused-parameter clean, we don't want to regress it.
-Wunused-but-set-variable is a close option in the family (though not under the -Wunused umbrella).
It seems ok to set related options together, even if llvm/ will get this option as well.
(Overriding a subset is probably confusing.)

Perhaps not many groups use standalone builds, so losing some warning options is fine.

llvm/'s warnings are under LLVM_ENABLE_WARNINGS, while libunwind specifics are unconditional. Now I am unsure what to do.

If this flag isn't turned on for non-standalone builds/turned on in LLVM, then I'm not sure what issue the patch is intending to address. There wouldn't be any inconsistency, there wouldn't be any regression issue (like some builds checking the warning and some not checking it - so someone in the non-checking build committing a warning violation, breaking the checking build) if the flag wasn't added?

But if someone wants to turn it on for the non-standalone build, that sounds OK. It's probably not especially painful to cleanup.

I support that llvm/cmake/modules/HandleLLVMOptions.cmake adds -Wunused-but-set-variable as well, after some cleanup.

[danielkiss]

But I'd object more to adding extra warnings per project without a pretty significant reason that one subproject should differ from others.

Flag is added here to have some regression check for the unused variables as this issue come up not just for me.
I'd keep the flag here until it is enable globally.

[dblaikie]

In D107835#2975142, @danielkiss wrote:

But I'd object more to adding extra warnings per project without a pretty significant reason that one subproject should differ from others.

Flag is added here to have some regression check for the unused variables as this issue come up not just for me.
I'd keep the flag here until it is enable globally.

I'm not sure I understand - what do you mean by "regression" - causing a build to fail because that build had the warning enabled already? Or you mean introducing code that has set-but-unused variables, but without any build failures?

I'd prefer we not enable this warning only for one subproject like this - divergence between subprojects increases maintenance burden and developer flexibility when moving between subprojects.

[danielkiss]

introducing code that has set-but-unused variables

This what I mean, code is unused variable free and the flag helps to stay as it is.

increases maintenance burden and developer flexibility when moving between subprojects.

I hope this flag is not so harmful nor problematic for new code.

I support that llvm/cmake/modules/HandleLLVMOptions.cmake adds -Wunused-but-set-variable as well, after some cleanup.

Sounds good to me.

[MaskRay]

-Wall enables -Wunused-but-set-variable, so the CMake change is not needed.

[danielkiss]

In D107835#2978584, @MaskRay wrote:

-Wall enables -Wunused-but-set-variable, so the CMake change is not needed.

oh right, dropped.

libunwind/CMakeLists.txt

 add_compile_flags_if_supported(-Wsign-compare)
 add_compile_flags_if_supported(-Wsign-conversion)
 add_compile_flags_if_supported(-Wstrict-aliasing=2)
 add_compile_flags_if_supported(-Wstrict-overflow=4)
 add_compile_flags_if_supported(-Wunused-parameter)
 add_compile_flags_if_supported(-Wunused-variable)
 add_compile_flags_if_supported(-Wwrite-strings)
 add_compile_flags_if_supported(-Wundef)
+add_compile_flags_if_supported(-Wunused-but-set-variable)
 
 add_compile_flags_if_supported(-Wno-suggest-override)
 
 if (WIN32)
   # The headers lack matching dllexport attributes (_LIBUNWIND_EXPORT);
   # silence the warning instead of cluttering the headers (which aren't
   # necessarily the ones that the callers will use anyway) with the
   # attributes.

libunwind/src/Unwind-EHABI.cpp

for (bool handlerNotFound = true; handlerNotFound;) {
     if (__unw_get_proc_info(cursor, &frameInfo) != UNW_ESUCCESS) {
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase1(ex_ojb=%p): __unw_get_proc_info "
           "failed => _URC_FATAL_PHASE1_ERROR",
           static_cast<void *>(exception_object));
       return _URC_FATAL_PHASE1_ERROR;
     }
 
+#ifndef NDEBUG
     // When tracing, print state information.
     if (_LIBUNWIND_TRACING_UNWINDING) {
       char functionBuf[512];
       const char *functionName = functionBuf;
       unw_word_t offset;
       if ((__unw_get_proc_name(cursor, functionBuf, sizeof(functionBuf),
                                &offset) != UNW_ESUCCESS) ||
           (frameInfo.start_ip + offset > frameInfo.end_ip))
         functionName = ".anonymous.";
       unw_word_t pc;
       __unw_get_reg(cursor, UNW_REG_IP, &pc);
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase1(ex_ojb=%p): pc=0x%" PRIxPTR ", start_ip=0x%" PRIxPTR ", func=%s, "
           "lsda=0x%" PRIxPTR ", personality=0x%" PRIxPTR,
           static_cast<void *>(exception_object), pc,
           frameInfo.start_ip, functionName,
           frameInfo.lsda, frameInfo.handler);
     }
+#endif
 
     // If there is a personality routine, ask it if it will want to stop at
     // this frame.
     if (frameInfo.handler != 0) {
       _Unwind_Personality_Fn p =
           (_Unwind_Personality_Fn)(long)(frameInfo.handler);
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase1(ex_ojb=%p): calling personality function %p",
while (true) {
     if (__unw_get_proc_info(cursor, &frameInfo) != UNW_ESUCCESS) {
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase2(ex_ojb=%p): __unw_get_proc_info "
           "failed => _URC_FATAL_PHASE2_ERROR",
           static_cast<void *>(exception_object));
       return _URC_FATAL_PHASE2_ERROR;
     }
 
+#ifndef NDEBUG
     // When tracing, print state information.
     if (_LIBUNWIND_TRACING_UNWINDING) {
       char functionBuf[512];
       const char *functionName = functionBuf;
       unw_word_t offset;
       if ((__unw_get_proc_name(cursor, functionBuf, sizeof(functionBuf),
                                &offset) != UNW_ESUCCESS) ||
           (frameInfo.start_ip + offset > frameInfo.end_ip))
         functionName = ".anonymous.";
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase2(ex_ojb=%p): start_ip=0x%" PRIxPTR ", func=%s, sp=0x%" PRIxPTR ", "
           "lsda=0x%" PRIxPTR ", personality=0x%" PRIxPTR "",
           static_cast<void *>(exception_object), frameInfo.start_ip,
           functionName, sp, frameInfo.lsda,
           frameInfo.handler);
     }
+#endif
 
     // If there is a personality routine, tell it we are unwinding.
     if (frameInfo.handler != 0) {
       _Unwind_Personality_Fn p =
           (_Unwind_Personality_Fn)(intptr_t)(frameInfo.handler);
       struct _Unwind_Context *context = (struct _Unwind_Context *)(cursor);
       // EHABI #7.2
       exception_object->pr_cache.fnstart = frameInfo.start_ip;
while (true) {
     unw_proc_info_t frameInfo;
     if (__unw_get_proc_info(cursor, &frameInfo) != UNW_ESUCCESS) {
       _LIBUNWIND_TRACE_UNWINDING("unwind_phase2_forced(ex_ojb=%p): __unw_step "
                                  "failed => _URC_END_OF_STACK",
                                  (void *)exception_object);
       return _URC_FATAL_PHASE2_ERROR;
     }
 
+#ifndef NDEBUG
     // When tracing, print state information.
     if (_LIBUNWIND_TRACING_UNWINDING) {
       char functionBuf[512];
       const char *functionName = functionBuf;
       unw_word_t offset;
       if ((__unw_get_proc_name(cursor, functionBuf, sizeof(functionBuf),
                                &offset) != UNW_ESUCCESS) ||
           (frameInfo.start_ip + offset > frameInfo.end_ip))
         functionName = ".anonymous.";
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase2_forced(ex_ojb=%p): start_ip=0x%" PRIxPTR
           ", func=%s, lsda=0x%" PRIxPTR ", personality=0x%" PRIxPTR,
           (void *)exception_object, frameInfo.start_ip, functionName,
           frameInfo.lsda, frameInfo.handler);
     }
+#endif
 
     // Call stop function at each frame.
     _Unwind_Action action =
         (_Unwind_Action)(_UA_FORCE_UNWIND | _UA_CLEANUP_PHASE);
     _Unwind_Reason_Code stopResult =
         (*stop)(1, action, exception_object->exception_class, exception_object,
                 (_Unwind_Context *)(cursor), stop_parameter);
     _LIBUNWIND_TRACE_UNWINDING(

libunwind/src/Unwind-seh.cpp

while (__unw_step(&cursor2) > 0) {
     unw_proc_info_t frameInfo;
     if (__unw_get_proc_info(&cursor2, &frameInfo) != UNW_ESUCCESS) {
       _LIBUNWIND_TRACE_UNWINDING("unwind_phase2_forced(ex_ojb=%p): __unw_step "
                                  "failed => _URC_END_OF_STACK",
                                  (void *)exception_object);
       return _URC_FATAL_PHASE2_ERROR;
     }
 
+#ifndef NDEBUG
     // When tracing, print state information.
     if (_LIBUNWIND_TRACING_UNWINDING) {
       char functionBuf[512];
       const char *functionName = functionBuf;
       unw_word_t offset;
       if ((__unw_get_proc_name(&cursor2, functionBuf, sizeof(functionBuf),
                                &offset) != UNW_ESUCCESS) ||
           (frameInfo.start_ip + offset > frameInfo.end_ip))
         functionName = ".anonymous.";
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase2_forced(ex_ojb=%p): start_ip=0x%" PRIx64
           ", func=%s, lsda=0x%" PRIx64 ", personality=0x%" PRIx64,
           (void *)exception_object, frameInfo.start_ip, functionName,
           frameInfo.lsda, frameInfo.handler);
     }
+#endif
 
     // Call stop function at each frame.
     _Unwind_Action action =
         (_Unwind_Action)(_UA_FORCE_UNWIND | _UA_CLEANUP_PHASE);
     _Unwind_Reason_Code stopResult =
         (*stop)(1, action, exception_object->exception_class, exception_object,
                 (struct _Unwind_Context *)(&cursor2), stop_parameter);
     _LIBUNWIND_TRACE_UNWINDING(

libunwind/src/UnwindLevel1.c

while (true) {
     if (__unw_get_proc_info(cursor, &frameInfo) != UNW_ESUCCESS) {
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase1(ex_ojb=%p): __unw_get_proc_info "
           "failed => _URC_FATAL_PHASE1_ERROR",
           (void *)exception_object);
       return _URC_FATAL_PHASE1_ERROR;
     }
 
+#ifndef NDEBUG
     // When tracing, print state information.
     if (_LIBUNWIND_TRACING_UNWINDING) {
       char functionBuf[512];
       const char *functionName = functionBuf;
       unw_word_t offset;
       if ((__unw_get_proc_name(cursor, functionBuf, sizeof(functionBuf),
                                &offset) != UNW_ESUCCESS) ||
           (frameInfo.start_ip + offset > frameInfo.end_ip))
         functionName = ".anonymous.";
       unw_word_t pc;
       __unw_get_reg(cursor, UNW_REG_IP, &pc);
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase1(ex_ojb=%p): pc=0x%" PRIxPTR ", start_ip=0x%" PRIxPTR
           ", func=%s, lsda=0x%" PRIxPTR ", personality=0x%" PRIxPTR "",
           (void *)exception_object, pc, frameInfo.start_ip, functionName,
           frameInfo.lsda, frameInfo.handler);
     }
+#endif
 
     // If there is a personality routine, ask it if it will want to stop at
     // this frame.
     if (frameInfo.handler != 0) {
       _Unwind_Personality_Fn p =
           (_Unwind_Personality_Fn)(uintptr_t)(frameInfo.handler);
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase1(ex_ojb=%p): calling personality function %p",
while (true) {
     if (__unw_get_proc_info(cursor, &frameInfo) != UNW_ESUCCESS) {
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase2(ex_ojb=%p): __unw_get_proc_info "
           "failed => _URC_FATAL_PHASE1_ERROR",
           (void *)exception_object);
       return _URC_FATAL_PHASE2_ERROR;
     }
 
+#ifndef NDEBUG
     // When tracing, print state information.
     if (_LIBUNWIND_TRACING_UNWINDING) {
       char functionBuf[512];
       const char *functionName = functionBuf;
       unw_word_t offset;
       if ((__unw_get_proc_name(cursor, functionBuf, sizeof(functionBuf),
                                &offset) != UNW_ESUCCESS) ||
           (frameInfo.start_ip + offset > frameInfo.end_ip))
         functionName = ".anonymous.";
       _LIBUNWIND_TRACE_UNWINDING("unwind_phase2(ex_ojb=%p): start_ip=0x%" PRIxPTR
                                  ", func=%s, sp=0x%" PRIxPTR ", lsda=0x%" PRIxPTR
                                  ", personality=0x%" PRIxPTR,
                                  (void *)exception_object, frameInfo.start_ip,
                                  functionName, sp, frameInfo.lsda,
                                  frameInfo.handler);
     }
+#endif
 
     // If there is a personality routine, tell it we are unwinding.
     if (frameInfo.handler != 0) {
       _Unwind_Personality_Fn p =
           (_Unwind_Personality_Fn)(uintptr_t)(frameInfo.handler);
       _Unwind_Action action = _UA_CLEANUP_PHASE;
       if (sp == exception_object->private_2) {
         // Tell personality this was the frame it marked in phase 1.
while (__unw_step(cursor) > 0) {
     unw_proc_info_t frameInfo;
     if (__unw_get_proc_info(cursor, &frameInfo) != UNW_ESUCCESS) {
       _LIBUNWIND_TRACE_UNWINDING("unwind_phase2_forced(ex_ojb=%p): __unw_step "
                                  "failed => _URC_END_OF_STACK",
                                  (void *)exception_object);
       return _URC_FATAL_PHASE2_ERROR;
     }
 
+#ifndef NDEBUG
     // When tracing, print state information.
     if (_LIBUNWIND_TRACING_UNWINDING) {
       char functionBuf[512];
       const char *functionName = functionBuf;
       unw_word_t offset;
       if ((__unw_get_proc_name(cursor, functionBuf, sizeof(functionBuf),
                                &offset) != UNW_ESUCCESS) ||
           (frameInfo.start_ip + offset > frameInfo.end_ip))
         functionName = ".anonymous.";
       _LIBUNWIND_TRACE_UNWINDING(
           "unwind_phase2_forced(ex_ojb=%p): start_ip=0x%" PRIxPTR
           ", func=%s, lsda=0x%" PRIxPTR ", personality=0x%" PRIxPTR,
           (void *)exception_object, frameInfo.start_ip, functionName,
           frameInfo.lsda, frameInfo.handler);
     }
+#endif
 
     // Call stop function at each frame.
     _Unwind_Action action =
         (_Unwind_Action)(_UA_FORCE_UNWIND | _UA_CLEANUP_PHASE);
     _Unwind_Reason_Code stopResult =
         (*stop)(1, action, exception_object->exception_class, exception_object,
                 (struct _Unwind_Context *)(cursor), stop_parameter);
     _LIBUNWIND_TRACE_UNWINDING(