Re: Remove misleading XOR
Hi Timm, On Thu, Feb 18, 2021 at 08:41:01AM +0100, Timm Bäder wrote: > On 17/02/2021 17:49, Mark Wielaard wrote: > > > > I think both the comment and the warning message are a little > > misleading. > > > > The comment should really read "Change state from CLEANING to > > NO_RESIZING". You are right that NO_RESIZING is just zero, but still > > removing it seems to make the code less clear. > > > > All this is slightly confusing since it is doing "double" xor. > > First it does an OLD_STATE_BITS ^ NEW_STATE_BITS and then a > > atomic_fetch_xor of the result of that xor with the resizing_state > > variable, Which is expected to be OLD_STATE, so the bits that are left > > should be the NEW_STATE bits after the double xor. > > This is how I understood the code. Given that all of the > atomic_fetch_xor_explicit() calls in that file preceded by a comment > explaining what they do, it might be useful to have a function > change_resizing_state(htab, from, to, memory_order) instead and remove > the comments. That also works around the clang warning of course, but > those are not the only places where resizing_state is modified, so it's > weird in a different way. > > > Could you see if something like this works around the warning? > > > > diff --git a/lib/dynamicsizehash_concurrent.c > > b/lib/dynamicsizehash_concurrent.c > > index 2d53bec6..0283eea1 100644 > > --- a/lib/dynamicsizehash_concurrent.c > > +++ b/lib/dynamicsizehash_concurrent.c > > @@ -160,10 +160,10 @@ insert_helper (NAME *htab, HASHTYPE hval, TYPE val) > > } > > } > > -#define NO_RESIZING 0u > > -#define ALLOCATING_MEMORY 1u > > -#define MOVING_DATA 3u > > -#define CLEANING 2u > > +#define NO_RESIZING 0x0 > > +#define ALLOCATING_MEMORY 0x1 > > +#define MOVING_DATA 0x3 > > +#define CLEANING 0x2 > > Nope, same result. :( Hmmm. I am not sure why that doesn't work. What if you make them explicitly unsinged (adding u at the end). Or does it simply ignore the values and just warn because it sees the macro name and not an explicit number? I think I don't really understand anymore what this warning is warning about. Could you give an example of where this would flag something that we would like to fix? Maybe it is easiest to just suppress this warning, when is it enabled? Thanks, Mark
Re: Remove misleading XOR
On 24/02/2021 14:16, Mark Wielaard wrote:
Hmmm. I am not sure why that doesn't work. What if you make them
explicitly unsinged (adding u at the end). Or does it simply ignore
the values and just warn because it sees the macro name and not an
explicit number?
I think I don't really understand anymore what this warning is warning
about. Could you give an example of where this would flag something
that we would like to fix?
I found https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90885, which has a
few more use-cases (in the form of tweets but anyway). GCC might also
get this warning in the future.
For the clang implementation, see https://reviews.llvm.org/D63423
So it only seems to warn for 2 ^ x because that's a common error:
#define NO_RESIZING 0x0u
#define CLEANING0x2u
int main(void) {
unsigned p = CLEANING ^ NO_RESIZING;
unsigned j = NO_RESIZING ^ CLEANING;
unsigned k = 17 ^ 3;
}
This code only warns for the declaration of p.
Maybe it is easiest to just suppress this warning, when is it enabled?
That's fine with me, I just didn't want to add compiler-specific flags
but I guess we can just check for -Wno-xor-used-as-pow and add that when
the compiler supports it.
- Timm
--
Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Michael O'Neill, Tom Savage, Eric
Shander
Re: Remove misleading XOR
On 24/02/2021 14:46, Timm Bäder wrote: On 24/02/2021 14:16, Mark Wielaard wrote: Hmmm. I am not sure why that doesn't work. What if you make them explicitly unsinged (adding u at the end). Or does it simply ignore the values and just warn because it sees the macro name and not an explicit number? I think I don't really understand anymore what this warning is warning about. Could you give an example of where this would flag something that we would like to fix? I found https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90885, which has a few more use-cases (in the form of tweets but anyway). GCC might also get this warning in the future. For the clang implementation, see https://reviews.llvm.org/D63423 Looking at the code, it has an explicit "don't diagnose binary, hexadecimal, octal literals" comment. Your suggestion of changing to hexadecimal literals would work, but clang doesn't resolve the macros first, see https://godbolt.org/z/3EMYY6. -- Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Michael O'Neill, Tom Savage, Eric Shander
build-ids, .debug_sup and other IDs (Was: [PATCH] Handle DWARF 5 separate debug sections)
Hi, I am adding the elfutils and dwz mailinglists to CC because I think you stumbled upon a silent assumption debuginfod makes which would be good to make explicit (or maybe change?) Context is that dwz 0.15 (still not released yet) can now produce standardized DWARF Supplementary Files using a .debug_sup section when using --dwarf-5 -m multifile. See this bug report: https://sourceware.org/bugzilla/show_bug.cgi?id=27440 The full patch to support that in gdb is here: https://sourceware.org/pipermail/gdb-patches/2021-February/176508.html But what I like to discuss is this part of Tom's email: On Sun, Feb 21, 2021 at 04:18:10PM -0700, Tom Tromey wrote: > DWARF 5 standardized the .gnu_debugaltlink section that dwz emits in > multi-file mode. This is handled via some new forms, and a new > .debug_sup section. > > This patch adds support for this to gdb. It is largely > straightforward, I think, though one oddity is that I chose not to > have this code search the system build-id directories for the > supplementary file. My feeling was that, while it makes sense for a > distro to unify the build-id concept with the hash stored in the > .debug_sup section, there's no intrinsic need to do so. Tom is correct. Technically a supplemental (alt) file id isn't a build-id. But it does smell like one. It is a large globally unique identifier that can be expressed as hex string. And the GNU extension defining alt files for DWARF < 5 (and still with DWARF5 currently by default because no consumer yet supports .debug_sup) will put it in a .note.gnu.build-id NOTE section as GNU_BUILD_ID. This has the nice side-effect that a distro will most likely make it available as /usr/lib/debug/.build-id/xx/.debug file. And that "build-id" is how you request it from debuginfod (you request /buildid/BUILDID/debuginfo). Now technically that is cheating and confusing two concepts, the build-id and supplemental file id. But I was personally assuming we would extend it to also to other things like dwo IDs (which are again almost identical globally unique identifiers for files). There one question would be if you would get the .dwo file or the .dwp file in which is was embedded (I would vote for the second). One consequence of conflating all these IDs is that it isn't immediately clear what a debuginfod request for /buildid/BUILDID/executable should return (probably nothing). Or if /buildid/BUILDID/source/SOURCE/FILE requests also (should) work for other IDs than build-ids. Any opinions on whether we should split these concepts out and introduce separate request mechanisms per ID-kind, or simply assume a globally unique id is globally unique and we just clarify what it means to use a build-id, sup_checksum or dwo_id together with a request for an executable, debuginfo or source/file? Thanks, Mark
Re: build-ids, .debug_sup and other IDs (Was: [PATCH] Handle DWARF 5 separate debug sections)
Hi Mark, Context is that dwz 0.15 (still not released yet) can now produce standardized DWARF Supplementary Files using a .debug_sup section when using --dwarf-5 -m multifile. See this bug report: https://sourceware.org/bugzilla/show_bug.cgi?id=27440 Is there somewhere that I can lay my hands on a file containing a .debug_sup section and its corresponding supplimentary file ? I would like to test the binutils to make sure that they can support them too. Cheers Nick
Re: build-ids, .debug_sup and other IDs (Was: [PATCH] Handle DWARF 5 separate debug sections)
Hi Nick, On Wed, Feb 24, 2021 at 05:00:14PM +, Nick Clifton wrote: > > Context is that dwz 0.15 (still not released yet) can now produce > > standardized DWARF Supplementary Files using a .debug_sup section > > when using --dwarf-5 -m multifile. See this bug report: > > https://sourceware.org/bugzilla/show_bug.cgi?id=27440 > > Is there somewhere that I can lay my hands on a file containing a > .debug_sup section and its corresponding supplimentary file ? I > would like to test the binutils to make sure that they can support > them too. Currently you need dwz git trunk (hopefully we will do a real release by Monday?). The following will get you a .sup file for dwz itself: $ git clone git://sourceware.org/git/dwz.git $ cd dwz $ ./configure $ make $ cp dwz one $ cp dwz two $ dwz --dwarf-5 -m sup one two If you already process .gnu_debugaltlink then processing the .debug_sup shouldn't be too hard. Just don't expect there to be a .note.gnu.build-id. Also note that instead of DW_FORM_GNU_alt_ref and DW_FORM_GNU_alt_strp the one and two files will contain DW_FORM_ref_sup and DW_FORM_ref_strp. The formal description of the .debug_sup section can be found in section 7.3.6 DWARF Supplementary Object Files http://dwarfstd.org/doc/DWARF5.pdf Cheers, Mark
Re: Remove misleading XOR
On 24/02/2021 14:16, Mark Wielaard wrote:> Hmmm. I am not sure why that doesn't work. What if you make them explicitly unsinged (adding u at the end). Or does it simply ignore the values and just warn because it sees the macro name and not an explicit number? Another viable workaround is to just use variables instead of macros: https://godbolt.org/z/9zEEve -- Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Michael O'Neill, Tom Savage, Eric Shander
Re: build-ids, .debug_sup and other IDs
> "Mark" == Mark Wielaard writes: >> This patch adds support for this to gdb. It is largely >> straightforward, I think, though one oddity is that I chose not to >> have this code search the system build-id directories for the >> supplementary file. My feeling was that, while it makes sense for a >> distro to unify the build-id concept with the hash stored in the >> .debug_sup section, there's no intrinsic need to do so. Mark> Any opinions on whether we should split these concepts out and introduce Mark> separate request mechanisms per ID-kind, or simply assume a globally Mark> unique id is globally unique and we just clarify what it means to use Mark> a build-id, sup_checksum or dwo_id together with a request for an Mark> executable, debuginfo or source/file? FWIW I looked a little at unifying these. For example, bfdopncls.c:bfd_get_alt_debug_link_info could look at both the build-id and .debug_sup. But, this seemed a bit weird. What if both appear and they are different? Then a single API isn't so great -- you want to check the ID corresponding to whatever was in the original file. Probably I should have stuck some of the new code into BFD though. It's not too late to do that at least. I suppose a distro can ensure that the IDs are always equal. Maybe debuginfod could also just require this. Tom
