This is an archive of the discontinued LLVM Phabricator instance.

[libc] Fix reusing/redefining `FPBits` symbols.
AbandonedPublic

Authored by lntue on Jan 30 2023, 11:21 AM.

Details

Summary

Change all of FPBits re-declaration to FPB instead.
See https://github.com/llvm/llvm-project/issues/57719#issuecomment-1333900338

Diff Detail

Event Timeline

lntue created this revision.Jan 30 2023, 11:21 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 30 2023, 11:21 AM
lntue requested review of this revision.Jan 30 2023, 11:21 AM
This revision is now accepted and ready to land.Jan 31 2023, 3:11 PM

Can you please hold off on this change. I will update here when it is ready.

Can you explain how some of the use cases here are different from https://godbolt.org/z/arvG69d79.

lntue added a comment.Jan 31 2023, 7:57 PM

Can you explain how some of the use cases here are different from https://godbolt.org/z/arvG69d79.

I'm not so sure why there were warnings by gcc in the github issue, but not in godbolt example. My guess is that they are from the constructors:

  • Without using FPBits = FPBits<float>, something like FPBits(1.0) will be understood as FPBits<double>(1.0)
  • With using FPBits = FPBits<float>, FPBits(1.0) now will be FPBits<float>(1.0) from implicit conversion.

These warnings seem to be inconsistent between different compilers and maybe different versions, but I think it's good to clean them up to prevent ambiguity.

sivachandra added a comment.EditedJan 31 2023, 9:32 PM

I think the best understanding of the problem comes from these two links:

clang allows it: https://godbolt.org/z/z5rWhzKET
gcc complains: https://godbolt.org/z/T6dvxTWKv

GCC's error message is actually very suggestive. IIUC, it is saying that we are changing the meaning of the type named S within the scope of the U (it is easy to see the problem if U were the global namespace). I think the older version of the code linked to in the bug had this problem: https://github.com/llvm/llvm-project/blob/d883a4ad02d867e7037bd4ec342016c402484148/libc/src/__support/FPUtil/except_value_utils.h

IMHO, it is very subtle. The problem code redefined the type names within the namespace __llvm_libc::fputil. But, I think the current code does not have any such instances - the renames are happening in the namespace __llvm_libc. So, my guess is that that is why we are not seeing similar errors with GCC anymore. Considering that we do have a GCC bot, do we really want to "fix"?

lntue added a comment.Feb 1 2023, 8:02 AM

I think the best understanding of the problem comes from these two links:

clang allows it: https://godbolt.org/z/z5rWhzKET
gcc complains: https://godbolt.org/z/T6dvxTWKv

GCC's error message is actually very suggestive. IIUC, it is saying that we are changing the meaning of the type named S within the scope of the U (it is easy to see the problem if U were the global namespace). I think the older version of the code linked to in the bug had this problem: https://github.com/llvm/llvm-project/blob/d883a4ad02d867e7037bd4ec342016c402484148/libc/src/__support/FPUtil/except_value_utils.h

IMHO, it is very subtle. The problem code redefined the type names within the namespace __llvm_libc::fputil. But, I think the current code does not have any such instances - the renames are happening in the namespace __llvm_libc. So, my guess is that that is why we are not seeing similar errors with GCC anymore. Considering that we do have a GCC bot, do we really want to "fix"?

I think you're right! As long as the gcc bot is not complaining, this patch is not needed.

lntue abandoned this revision.Feb 1 2023, 8:03 AM